Share regex_remap rule sets

[bneradt]

Reloading remap.config becomes slow when it contains many mappings that
reference the same regex_remap rule files. Since the PCRE2 conversion,
every plugin instance JIT-compiles an independent copy, making reload
time scale with instances rather than unique rule sets.

This patch caches immutable compiled rule sets by resolved filename and
exact source content. It keeps match contexts and profiling counters per
instance and uses weak ownership so obsolete reload generations are
released, preserving JIT request performance without redundant reload
work.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

plugins/regex_remap/regex_remap.cc:914

• Reading the rule file into a pre-sized string using the earlier path stat size can spuriously fail with a "short read" if the file is replaced between stat() and open() (e.g., atomic rewrite + rename), aborting remap.config reload even though reading to EOF would succeed. Prefer reading the whole stream and using the actual bytes read as the cache key.

  std::streamsize const expected_size = st.st_size;
  std::string           source(static_cast<size_t>(expected_size), '\0');
  f.read(source.data(), expected_size);
  if (f.bad() || f.gcount() != expected_size) {
    TSError("[%s] short read on %s: got %lld of %lld bytes", PLUGIN_NAME, ri->filename.c_str(), static_cast<long long>(f.gcount()),
            static_cast<long long>(expected_size));
    return TS_ERROR;

[cmcfarlen]

The design is right and the sharing boundary is drawn in the correct place. Since "what may be shared" is the whole question here, I verified that side rather than just reading it.

The regression premise checks out, and it is in the shipped 10.2.0. #12685 ("Remove PCRE references, complete migration to PCRE2") is on both master and 10.2.x, and Regex::compile() unconditionally runs pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) (Regex.cc:446). So every instance really did pay a full JIT compile per rule, and reload cost really did scale with mapping count rather than unique rule files.

Sharing the compiled rules is safe, and the match-context change was not optional. Previously RemapRegex held RegexMatchContext const *_match_context pointing at one RemapInstance's context. Once a RemapRegex is shared by every instance that loaded the same file, that member becomes flatly wrong — whichever instance compiled it last would own the pointer everyone uses. Passing the context as an argument is the necessary fix, not a stylistic one, and the class comment spelling out "nothing per-instance or per-transaction may be stored here" is the kind of note that will actually prevent the next regression.

I also confirmed the concurrent read is safe rather than assuming it. RegexMatchContext owns only a bare pcre2_match_context for the match limit — no JIT stack. JIT stacks live exclusively in RegexContext, which is thread_local RegexContext ctx (Regex.cc:89), so each ET_NET thread has its own. That matters because a PCRE2 JIT stack must not be used by two threads at once; if RegexMatchContext had carried one, the pre-existing per-instance sharing would already have been a problem and this patch would have widened it. It doesn't, so a pcre2_match_context read concurrently is fine, and the compiled pcre2_code is documented as safe for concurrent matching.

The weak ownership does what the comment claims. The cache stores std::weak_ptr<RuleSet const> and never extends a lifetime, entry.lock() cannot hand back an expired generation so the hit path needs no separate liveness check, and pruning is genuinely just bookkeeping. Compiling under the lock is the right trade: it serializes duplicate work instead of racing several instances into compiling the same source.

One thing I would change. rule_hits is sized only when profiling is on:

if (ri->profile) {
  ri->rule_hits.resize(ri->rule_set->rules().size());
}

and indexed only when profiling is on:

if (ri->profile) {
  ink_atomic_increment(&(ri->rule_hits[rule_ix]), 1);

That is correct today — profile is set during argument parsing and never changes afterward, so the two guards cannot disagree. But the invariant is implicit and split across TSRemapNewInstance and TSRemapDoRemap, and the failure mode if it is ever broken is not a null check away: ink_atomic_increment on &rule_hits[ix] of an empty vector is an out-of-bounds atomic write on the heap, from every ET_NET thread, on every matching request. Given the class comment already warns the next person to add per-instance state "indexed in lockstep with RuleSet::rules()", it seems worth making that unconditional — resizing always costs a handful of ints per instance, and it removes the coupling entirely. Failing that, an assert on the size next to the increment would at least make a debug build say so.

Smaller notes, none blocking:

• The cache mutex is a single global one, so a compile of file A blocks a compile of file B. Remap loading is single-threaded, so this is uncontended in practice and costs nothing — but that also means the "concurrent instances of the same source must serialize here" comment describes a case that may not currently arise. Harmless either way; just do not count on it for parallelism if remap loading is ever parallelized.
• The key is the resolved filename (absolute, or prefixed with TSConfigDirGet()), but not canonicalized: a symlink, a .., or a ./ prefix yields a distinct key. Because the content is compared too, the only consequence is a missed share and one redundant compile, never a wrong rule set. Fine as-is; worth knowing it is path-string equality rather than same-file identity.
• The stat() / read pair handles the TOCTOU correctly by comparing gcount() against the stat size, so a file rewritten between the two is an error rather than a truncated rule set. Good.

On the test. Asserting via grep -c on the regex_remap debug output is brittle in the usual ways — it depends on debug tags staying enabled and on the exact Dbg wording — and the file is honest about the first of those. Given there is no metric or RPC exposing cache hits, I do not have a better suggestion, and checking both a shared and a deliberately isolated rule file plus a post-reload generation is the right set of cases. Just be aware that rewording either debug line silently turns these assertions into no-ops that still pass the redirect checks.

Backport. This is already tracked for 10.2.x, and it should pick cleanly: the new test uses Test.AddConfigReload(..., expect_tasks=["remap.config"]) and the await-file helper, both of which are on 10.2.x (the former via #13502). Since the regression shipped in rc0, landing it there seems right.

Nothing blocking from me.

[cmcfarlen]

I also read this (not just Claude). I couldn't tell if the cache would ever actually be used concurrently, but it is global so a mutex makes sense. I always enjoy seeing std::weak_ptr in the wild. Cool!

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 3ac694d for the 10.2.0 release.