You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/11 23:08:16 UTC
[PR] Share regex_remap rule sets [trafficserver]
bneradt opened a new pull request, #13537:
URL: https://github.com/apache/trafficserver/pull/13537
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Share regex_remap rule sets [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13537:
URL: https://github.com/apache/trafficserver/pull/13537#issuecomment-5260275318
[approve ci freebsd autest 2]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Share regex_remap rule sets [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13537:
URL: https://github.com/apache/trafficserver/pull/13537#issuecomment-5269535563
Cherry-picked to the 10.2.x branch as 3ac694d766844524a5bc49a6671f96092ca7fb27 for the 10.2.0 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Share regex_remap rule sets [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13537:
URL: https://github.com/apache/trafficserver/pull/13537#issuecomment-5260522750
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:
```cpp
if (ri->profile) {
ri->rule_hits.resize(ri->rule_set->rules().size());
}
```
and indexed only when profiling is on:
```cpp
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Share regex_remap rule sets [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13537:
URL: https://github.com/apache/trafficserver/pull/13537
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org