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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
