Skip to content

benchmark: measure Regex time and heap allocations - #13670

Open
bryancall wants to merge 10 commits into
apache:masterfrom
bryancall:regex-benchmark-regex
Open

benchmark: measure Regex time and heap allocations#13670
bryancall wants to merge 10 commits into
apache:masterfrom
bryancall:regex-benchmark-regex

Conversation

@bryancall

@bryancall bryancall commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

There was no way to answer "is this faster" or "does this allocate more" about a change to
the regex wrapper except by writing a throwaway program each time. This adds a permanent
one.

What it measures

The corpus is what the tree actually matches: a remap path rule, a host allowlist, an
extension test, and the crash-guard rule from #5762.

Time for compiling, copying, a boolean match, a match with captures both with a fresh
matches object and a reused one, matching through the shared context and through a caller
supplied one, a twenty pattern DFA scan, and the long subject that drives a match to the
JIT stack bound.

Heap allocations per operation, counted by interposing the system allocator. PCRE2
makes every allocation for a compile or a match through the callbacks this wrapper
installs, and those reach the system allocator, so the count covers what the wrapper
caused rather than only what it asked for directly.

Results on the current tree

Measured on a Ryzen 9 9950X3D, Fedora 44, gcc 16.2.1, PCRE2 10.47, medians of three rounds
of 50 samples:

operation time allocations
bool exec, hit 25.7 ns 0
bool exec, miss 18.3 ns 0
exec with captures, hit 25.6 ns 0
exec with captures, reused matches 19.8 ns 0
compile a path pattern 3017 ns 6, 8813 bytes
copy a compiled pattern 7.4 ns 1, 197 bytes
DFA match, first of 20 26.5 ns 0
DFA match, last of 20 781 ns 0
exec that exhausts the JIT stack 51.0 us 0

Every figure in that table is produced by this file, timings from the [bench] cases and
allocation counts from the [alloc] case, so the whole thing is reproducible rather than
transcribed. The allocation column is measured on Linux; on a platform without the
interposer the run reports the counts as unavailable rather than printing zero.

Two things worth recording. Matching already reaches the heap zero times, because
RegexMatches hands the pcre2 general context and match data out of its own 400 byte
buffer; any future change to this class has to preserve that rather than rediscover it.
And a copy is 400 times cheaper than a compile, which is not a free lunch: see #13671,
where it turns out the copy is cheap because pcre2_code_copy() leaves the just-in-time
machine code behind.

Building and running

Two targets, both behind -DENABLE_BENCHMARKS=ON:

cmake --build <build> --target benchmark_Regex benchmark_Regex_alloc
./<build>/tools/benchmark/benchmark_Regex "[bench]" --benchmark-samples 50
./<build>/tools/benchmark/benchmark_Regex_alloc "[alloc]"

They are one source file. The timing target has no allocator interposition compiled into
it, so a timed case measures the regex path alone: measured against a control build, a
wrapper call costs about 1.5 ns, which is noise on a 3,000 ns compile but a fifth of a 7.4 ns
copy. The allocation target carries the interposer and times nothing.

Notes

  • Built only with -DENABLE_BENCHMARKS=ON, which is off by default, so this changes
    nothing about a normal build.
  • Numbers are reported, not asserted. Block sizes differ between PCRE2 releases, so an
    assertion on allocation counts would fail on a version whose match data does not fit the
    inline buffer, which is a fact worth printing rather than a test worth failing.
  • The allocation counters are wired up on Linux, where defining the allocator symbols in
    the executable is enough to interpose them. Elsewhere they report themselves unavailable
    rather than printing zero.

There was no way to answer "is this faster" or "does this allocate more" about a
change to the regex wrapper except by writing a throwaway program each time.

The corpus is what the tree actually matches: a remap path rule, a host
allowlist, an extension test, and the crash-guard rule from apache#5762. It times
compiling, copying, a boolean match, a match with captures both with a fresh
matches object and a reused one, matching through the shared context and through
a caller supplied one, a twenty pattern set scan, and the long subject that
drives a match onto the interpreter.

The allocation half counts calls to the system allocator across each operation.
PCRE2 makes every allocation for a compile or a match through the callbacks this
wrapper installs, and those reach the system allocator, so the count covers what
the wrapper caused. A match under the just-in-time engine should not reach it at
all; the interpreter allocates a frames vector and does not. The counters are
wired up on Linux, where defining the allocator symbols in the executable is
enough to interpose them, and report themselves unavailable elsewhere rather
than printing zero.

Numbers are reported, not asserted. Block sizes differ between PCRE2 releases,
so an assertion here would fail on a version whose match data does not fit the
inline buffer, which is a fact worth printing rather than a test worth failing.
Copilot AI lite review requested due to automatic review settings September 12, 2026 19:26
@bryancall bryancall self-assigned this Sep 12, 2026
@bryancall bryancall added this to the 11.0.0 milestone Sep 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved allocator safety and benchmark correctness issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an opt-in Catch2 Regex benchmark for timing and heap-allocation measurements.

Changes:

  • Adds Regex compile, match, DFA, and JIT-stack benchmarks.
  • Adds Linux allocator interposition and allocation reporting.
  • Registers the benchmark target in CMake.
File summaries
File Summary and findings
tools/benchmark/CMakeLists.txt Builds and links the Regex benchmark.
tools/benchmark/benchmark_Regex.cc Implements the benchmarks and allocation measurements. Findings: critical allocator-resolution safety issue (3 votes); moderate timed initialization issue (1 vote); moderate interpreter-path labeling issue (1 vote); nit incomplete documentation sentence (1 vote).
Review details

Suppressed comments (4)

tools/benchmark/benchmark_Regex.cc:158

  • <cstdlib> declares the libc allocation functions as non-throwing on the Linux toolchains this target supports, but these interposed definitions omit noexcept. GCC therefore diagnoses the definitions as conflicting exception specifications (the same applies to free, calloc, and realloc below), so benchmark_Regex does not compile. Add matching noexcept to all four wrappers or avoid redeclaring these C APIs in C++.
extern "C" void *
malloc(size_t size)

tools/benchmark/benchmark_Regex.cc:10

  • This sentence is incomplete: and does not has no object, so the new file's introductory documentation ends with an unfinished claim. Complete it by stating that the interpreter allocates a vector of frames instead.
  allocator zero times; the interpreter allocates a frames vector and does not.

tools/benchmark/benchmark_Regex.cc:320

  • The function-local static is initialized on the first invocation of the timed benchmark body, so that sample includes RegexMatches/PCRE2 match-data construction rather than measuring only reuse. Construct the matches object before BENCHMARK so setup is outside the timed region.
  BENCHMARK("exec with captures, reused matches object")
  {
    static RegexMatches matches;
    return path.exec(SUBJECT_PATH, matches);
  };

tools/benchmark/benchmark_Regex.cc:394

  • This case is labeled as the interpreter path, but Regex::compile() JIT-compiles the pattern and exec() simply calls pcre2_match(); it never selects the interpreter. The timing and allocation result therefore measures the JIT stack-limit path, not interpreter backtracking frames as the surrounding comments claim. Rename/document this as a JIT stack-limit case, or add a separate benchmark that can actually disable JIT.
TEST_CASE("Regex interpreter path", "[bench][regex]")
{
  // A subject long enough to exhaust the JIT stack for this pattern, so the operation
  // measured is the error return rather than a match. This is the shape the crash guard
  // from #5762 covers, and it is the one place a match is expected to cost real time.
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tools/benchmark/benchmark_Regex.cc Outdated
resolve_real_allocators() assigned real_malloc first and returned early on it,
so a nested allocation during the remaining dlsym lookups re-entered a wrapper,
found the early return satisfied, and called through a still-null real_free.

Resolve all four into locals and publish them together with real_malloc last,
which is the pointer the early return tests. Each wrapper keeps testing its own
pointer, so none can reach a half-resolved table: free leaks rather than calling
null while resolving, and realloc moves a bootstrap block with malloc and memcpy
instead of handing a pointer the system allocator never issued to the real
realloc.
Copilot AI review requested due to automatic review settings September 12, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical build and allocator-safety issues, along with benchmark correctness concerns, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

tools/benchmark/benchmark_Regex.cc:10

  • This sentence is incomplete: it ends with “does not” without saying what the JIT does not do, so the allocation behavior being documented is unclear.

tools/benchmark/benchmark_Regex.cc:178

  • This executable interposes malloc/calloc/realloc on Linux even when alloc_counting is false, so the compile and copy timing benchmarks still include the extra wrapper branch and indirect call for every allocation. Those results are not timings of the normal Regex path, and the distortion changes with the number of allocations; isolate allocation counting in a separate executable or otherwise keep the interposer out of timing runs.
  record(size);
  return real_malloc(size);

tools/benchmark/benchmark_Regex.cc:535

  • Regex::compile() unconditionally calls pcre2_jit_compile(), and when the JIT stack is exhausted pcre2_match() returns PCRE2_ERROR_JIT_STACKLIMIT rather than falling back to the interpreter. Therefore the allocation loop below does not measure the interpreter's frame allocation as this comment claims; label it as the JIT-stack-limit case, or add an explicit non-JIT execution path before reporting interpreter allocations.
    // The interpreter allocates its backtracking frames through the match data's
    // allocator; the JIT engine does not. This is the operation that separates them.

tools/benchmark/benchmark_Regex.cc:182

  • <cstdlib> declares ::free with a non-throwing exception specification. Defining it without noexcept is an incompatible redeclaration in C++20, so this benchmark target will not compile.
free(void *p)

tools/benchmark/benchmark_Regex.cc:199

  • <cstdlib> declares ::calloc with a non-throwing exception specification. Defining it without noexcept is an incompatible redeclaration in C++20, so this benchmark target will not compile.
calloc(size_t n, size_t size)

tools/benchmark/benchmark_Regex.cc:216

  • <cstdlib> declares ::realloc with a non-throwing exception specification. Defining it without noexcept is an incompatible redeclaration in C++20, so this benchmark target will not compile.
realloc(void *p, size_t size)

tools/benchmark/benchmark_Regex.cc:37

  • std::max_align_t is used by the Linux allocator shim below, but this file never includes <cstddef>. <cstdlib> does not portably provide the std::max_align_t declaration, so enabling benchmarks can fail to compile on standard-library implementations that do not expose it transitively; include the header that owns the type.
#include <cstdio>
#include <cstdlib>
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread tools/benchmark/benchmark_Regex.cc Outdated
Comment thread tools/benchmark/benchmark_Regex.cc Outdated
…otstrap blocks

Three corrections to the allocator interposition.

glibc declares malloc, free, calloc and realloc with __THROW, which is noexcept
in C++, so defining them without it is an incompatible redeclaration. The
interposer is inside a Linux guard and glibc is what it is compiled against, so
the definitions now carry noexcept to match.

A bootstrap block now begins with a header holding its size, one max_align_t
wide so the pointer handed back keeps malloc's alignment. Without it a realloc
of a bootstrap block could not know how much to carry over.

realloc of a bootstrap block copies the old contents on both paths. It
previously returned a fresh block with nothing in it when the real allocator was
not resolved yet, and guessed at the length when it was. The loader reallocs
while resolving symbols, so returning uninitialised storage there fails the
lookup in a way that is very hard to read back to this function.

Also finish the sentence in the file comment describing which engine allocates.
Copilot AI review requested due to automatic review settings September 12, 2026 20:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Two moderate findings remain in benchmark_Regex.cc.

Review details

Suppressed comments (3)

tools/benchmark/benchmark_Regex.cc:455

  • This new test case is labeled as an interpreter benchmark, but Regex::compile() JIT-compiles the pattern and the body intentionally drives the JIT stack to its limit. Rename the case to describe the JIT-stack-limit operation; otherwise filtered benchmark output is misleading.
TEST_CASE("Regex interpreter path", "[bench][regex]")

tools/benchmark/benchmark_Regex.cc:38

  • std::max_align_t is used below, but this source does not include <cstddef>. The current <cstdlib>/standard-header include graph may provide it transitively on the tested libstdc++ builds, but that is not guaranteed, so the Linux benchmark can fail to compile with another standard library. Include the header that owns the declaration explicitly.
#include <cstdio>

tools/benchmark/benchmark_Regex.cc:384

  • Catch2 invokes this lambda for every timed iteration, so the function-local static leaves the thread-safe static-initialization guard on every call. The reported value therefore includes guard overhead instead of measuring only a reused RegexMatches; declare the object before BENCHMARK and capture it by reference.
  BENCHMARK("exec with captures, reused matches object")
  {
    static RegexMatches matches;
    return path.exec(SUBJECT_PATH, matches);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@bryancall bryancall closed this Sep 12, 2026
@bryancall bryancall reopened this Sep 12, 2026
…s guard

Three corrections.

The case called "Regex interpreter path" does not measure the interpreter.
compile() hands the pattern to the JIT, and what the long subject reaches is the
JIT's own stack bound, so the name made filtered output misleading. It is now
"Regex match that exhausts the JIT stack", with a note that reaching the
interpreter needs a pattern the JIT refuses outright, which costs tens of
seconds per match and has no business in a timed suite.

The reused-matches case held its RegexMatches in a function-local static, and
Catch2 runs that body once per timed iteration, so every iteration paid for the
thread-safe initialisation guard. Build it outside the timed body instead, which
is what the case was meant to measure in the first place.

std::max_align_t is declared in <cstddef>, which this file relied on getting
transitively. Include it.
Copilot AI review requested due to automatic review settings September 12, 2026 20:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The critical allocation issue and moderate benchmark correctness issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

tools/benchmark/benchmark_Regex.cc:469

  • This stress case assumes that Regex::compile() produced JIT code, but the project only requires libpcre2-8 and Regex::compile() ignores pcre2_jit_compile() failures. On a PCRE2 build without JIT, the 64 KiB pathological subject runs through the interpreter instead, potentially making the benchmark extremely slow and no longer measuring JIT-stack exhaustion. Detect JIT availability and skip or replace this case when it is unavailable.
  // Note this is not the interpreter: compile() gives the pattern to the JIT, and what the
  // long subject reaches is the JIT's own stack bound. Driving a match onto the interpreter
  // needs a pattern the JIT refuses outright, which starts around 36KB of pattern text and
  // costs tens of seconds per match, so it has no place in a timed suite.
  Regex query;
  query.compile(PATTERN_QUERY);

tools/benchmark/benchmark_Regex.cc:293

  • SUBJECT_MISS is not a miss for PATTERN_PATH: /no/match/here/at/all satisfies ^/([^/]+)/([^/]+)/(.*)$. Consequently the benchmark labelled exec with captures, miss at lines 390-394 measures another successful capture instead of the intended failure case. Use a subject without the required second slash segment, such as /no/match.
std::string_view const SUBJECT_MISS      = "/no/match/here/at/all";

tools/benchmark/benchmark_Regex.cc:225

  • The fallback multiplies n * size without checking for overflow. An overflowing calloc request can therefore receive a small bootstrap block and be zeroed as if the request succeeded, and the same wrapped value is recorded after resolution. Check the product before using it (or delegate the overflowed request to the real allocator once it is available) so the interposer preserves calloc failure semantics.
      void *p = bootstrap_alloc(n * size);
      if (p != nullptr) {
        memset(p, 0, n * size);
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread tools/benchmark/benchmark_Regex.cc
Comment thread tools/benchmark/benchmark_Regex.cc Outdated
…there

Three fixes.

bootstrap_alloc() rounded the request up to alignment before testing it against
the remaining buffer, so a size near the top of the range wrapped to a small
payload, passed the capacity test, and returned storage far smaller than asked
for. These wrappers stand in for malloc for every library in the process while
dlsym resolves, so that block would have corrupted somebody else's startup.
Check the request against what is left first, then round. calloc got the same
treatment for its n * size, which could wrap and then memset a huge region
through a small block.

The two cases that drive a subject into the JIT stack bound now check that this
PCRE2 actually produced machine code for the pattern. Only libpcre2-8 is
required, and a build without the just-in-time compiler, or one that refuses this
pattern, would run the same subject to completion on the interpreter: much
slower, and measuring something else under the same label. The timed case skips
and the allocation case says so rather than reporting a number that means
something different.

Also correct the comment on the allocation case, which described it as
separating the interpreter from the JIT. It does not: it shows that giving up
against the JIT stack costs no allocations either.
Copilot AI review requested due to automatic review settings September 12, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved allocator-fallback and JIT-stack validation issues remain, and DFA allocation results are not reproducible.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tools/benchmark/benchmark_Regex.cc:596

  • The published allocation results include DFA match, first of 20 and DFA match, last of 20, but this allocation section never constructs or executes a DFA under CountAllocations; the DFA is only exercised by the timing test above. As written, rerunning this benchmark cannot produce those allocation rows, so either add counted DFA cases or remove them from the reported results.
  {
    constexpr unsigned long COMPILES = 1000;
    CountAllocations        counter;
    for (unsigned long i = 0; i < COMPILES; ++i) {
      Regex re;
      re.compile(PATTERN_PATH);
    }
    report_allocations("compile a path pattern", counter.stats(), COMPILES);
  }
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread tools/benchmark/benchmark_Regex.cc Outdated
Comment thread tools/benchmark/benchmark_Regex.cc Outdated
…oc honestly

Three fixes, one of which was a claim this benchmark could not back.

The published results carried allocation figures for the two DFA scans, and
nothing in the allocation section ever built or ran a DFA. The numbers happened
to be right, zero either way, but they were not reproducible from this file.
Count them, so the row means something.

The two cases that depend on the subject reaching the just-in-time engine's
stack bound asked whether PCRE2 had produced machine code, which does not
establish it. A release whose JIT uses the stack differently would simply
complete the match, and both cases would then measure an ordinary match under
the stack-exhaustion label. Run the match once and look for the stack limit
error instead, and build the subject in one place so the probe and the
measurement cannot drift apart.

realloc of a pointer that came from neither the bootstrap buffer nor a real
allocator, while resolution is still in progress, returned a fresh bootstrap
block. That is worse than failing: the caller takes uninitialised storage for
its moved data and the original goes unfreed. Return null, which is a documented
realloc outcome and leaves the caller's pointer valid.
Copilot AI review requested due to automatic review settings September 12, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix handling of realloc(nullptr, size) during allocator resolution.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread tools/benchmark/benchmark_Regex.cc
realloc is defined to behave as malloc when the pointer is null, and a caller is
entitled to use it that way. The wrapper checked the pointer against the
bootstrap buffer, found a null pointer was not from there, and fell through to
the unresolved-pointer path, which reports failure. While the resolver was still
running that turned a legitimate allocation into an unexpected failure for
whoever made it, which during dlsym is the loader.

Handle the null pointer first, by routing it through the interposed malloc.
Copilot AI review requested due to automatic review settings September 12, 2026 21:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical build and allocator-safety findings, plus moderate benchmark correctness and runtime findings, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

tools/benchmark/benchmark_Regex.cc:351

  • On a build without JIT support, or when this pattern is rejected by pcre2_jit_compile, this probe still executes the 64 KiB pathological subject through the interpreter before it can return false. The file notes that this can take tens of seconds per match, so both the [bench] case and allocation case can spend excessive time instead of reaching their skip output. Check that JIT code exists before this call, or otherwise bound the probe.
  return re.exec(subject, matches) == PCRE2_ERROR_JIT_STACKLIMIT;

tools/benchmark/benchmark_Regex.cc:450

  • SUBJECT_MISS is not a miss for PATTERN_PATH: /no/match/here/at/all has two non-slash components followed by a remainder, so this call succeeds. The benchmark therefore measures a capture hit while reporting it as a miss; use a subject that cannot satisfy the path pattern (for example SUBJECT_HOST) or a dedicated path-miss subject.
    return path.exec(SUBJECT_MISS, matches);
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread tools/benchmark/CMakeLists.txt Outdated
Comment thread tools/benchmark/benchmark_Regex.cc
"exec with captures, miss" ran the path pattern against "/no/match/here/at/all",
which the path pattern matches: a leading slash, two slash free components, and
"here/at/all" as the remainder, returning four. The case has been timing a hit
under the name of a miss, and it shows in the number, 15.7 ns against the 23.7 ns
it reported before. Give it a subject the pattern cannot satisfy and say in the
corpus why the two miss subjects differ. The host pattern's miss was a real miss
and is unchanged.

from_bootstrap() compared an arbitrary allocator pointer against an element of
the bootstrap buffer with relational operators, which C++ leaves without a
defined ordering for unrelated objects. Compare the addresses instead. A wrong
answer there is not harmless: a false positive makes free() leak the block and
makes realloc() read a header that was never written.

The JIT stack probe ran the long subject before it could return false, so on a
build without machine code for the pattern it paid the entire interpreter cost to
discover there was no JIT stack to exhaust. Ask PCRE2_INFO_JITSIZE first, which
is cheap and answers that case, and only then confirm the bound is reached.

Define _GNU_SOURCE for the target. g++ and clang++ already define it for C++ on
Linux, which is why this has been building, but the dependency on RTLD_NEXT
should not rest on that.
Copilot AI review requested due to automatic review settings September 12, 2026 21:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The allocator interposer remains active during timed cases and can distort allocation-heavy benchmark results.

Review details

Suppressed comments (1)

tools/benchmark/benchmark_Regex.cc:215

  • The interposer remains on for every [bench] case, so any timed compile or copy pays this extra branch/TLS counter access and an indirect call before reaching the real allocator. That distorts the allocation-heavy timings—especially the reported ~7 ns copy, where one wrapper call can dominate the operation—rather than measuring the Regex/PCRE2 path alone. Run timing cases in a target without the interposer (or otherwise bypass instrumentation during timing) and reserve this path for [alloc] measurements.
  record(size);
  return real_malloc(size);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The interposer was compiled into the same executable that runs the timed cases,
so every allocation a timed operation made paid for a branch, a thread local
read and an indirect call before reaching the real allocator. Measured by
building the file twice, once with the wrappers and once without, medians of
three rounds of 50 samples:

  compile path pattern       3026.32 ns   3058.20 ns   -31.88 ns
  compile host pattern       2620.10 ns   2626.90 ns    -6.80 ns
  compile extension pattern  3272.49 ns   3259.85 ns   +12.64 ns
  copy a compiled pattern       8.87 ns      7.37 ns    +1.49 ns

The compiles are noise at about one percent of the operation. The copy is not:
it makes a single allocation, one wrapper call costs about 1.5 ns, and the
figure was running a fifth high. Matching is unaffected either way because it
allocates nothing.

One source now builds two targets. benchmark_Regex has no interposer compiled
in, so a timed case measures the regex path and nothing else; nm confirms it
exports none of the four allocator symbols. benchmark_Regex_alloc is built with
BENCHMARK_REGEX_ALLOC, carries the interposer and runs only the counting cases,
where the overhead does not matter because nothing is timed.
Copilot AI review requested due to automatic review settings September 12, 2026 21:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two unresolved findings remain, including a critical allocator-safety issue.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tools/benchmark/benchmark_Regex.cc:422

  • The allocation target compiles and registers every timing TEST_CASE because this section is not excluded when BENCHMARK_REGEX_ALLOC is defined. Running benchmark_Regex_alloc without the documented [alloc] filter therefore executes the [bench] cases under the allocator interposer and reports timings from a target whose purpose is allocation counting, contradicting the target comment and making an unfiltered run misleading. Compile the timing section only for the non-allocation target (or otherwise make the allocation target register only the counting case).
TEST_CASE("Regex compile", "[bench][regex]")
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread tools/benchmark/benchmark_Regex.cc Outdated
Splitting the two targets left the timed cases compiled into both, so running
benchmark_Regex_alloc without the [alloc] filter ran them under the interposer
and reported timings from the binary built to count allocations. That is the
thing the split was for. The timed section is now excluded when
BENCHMARK_REGEX_ALLOC is defined: the timing target registers five cases and the
allocation target registers one, whatever filter is given.

Also publish the four resolved allocator pointers all together or not at all.
Each wrapper does check its own pointer before using it, so a partial table was
never dereferenced, but one invariant is easier to keep true than four, and a
dlsym failure means the safe fallback paths are where every wrapper should stay.
Copilot AI review requested due to automatic review settings September 12, 2026 21:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The benchmark and allocator-interposition changes warrant final human review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants