Conversation
|
Marking this a draft: as written it does not achieve the case that motivated it, and I would rather say so here than leave the PR body overstating it. Testing it in a consumer showed the pattern cache is bounded by Measured by scanning a local server with that template, varying the number of responses:
At 6144 it is marginally worse than upstream. Sized to the pattern set it works — upstream's allocation scales with responses while the cached version stays nearly flat — at a cost of roughly 130 MiB retained. Full detail in #322. I did not want to unilaterally pick a new default for a library-wide cache, so I will rework this once there is a steer on sizing: a separate larger default for patterns, unbounded, or configurable. Happy to take direction, or to close this if you would rather solve it differently. |
|
Update: revised and taken out of draft. My earlier note in #322 left this parked because I didn't want to guess at the cache size. Rather than leave it stalled, I've gone with option 3 from that comment — a dedicated, exported default sized to the pattern working set:
The updated PR description carries the sizing table and rationale. The default is a one-line change — happy to go more conservative (e.g. |
regex, regex_all, regex_any and replace_regex compiled their pattern on every invocation. The pattern is normally a constant in the expression while the subject changes between evaluations, so the same pattern was compiled over and over — and compilation is the expensive part of these helpers, depending only on the first argument. The existing result cache does not cover this: its key hashes the function name and every argument, so a differing subject misses the cache and the pattern is compiled again. The caching is at call granularity while the reusable work is at pattern granularity. Compiled patterns now live in a cache keyed on the pattern, bounded by DefaultCacheSize like resultCache so patterns assembled at runtime cannot grow it without limit. *regexp.Regexp is safe for concurrent use, so one compilation serves every caller. Benchmarked with a constant pattern against a unique ~19KB subject per iteration, so the result cache cannot hit: 28182 B/op and 60 allocs/op before, 19219 B/op and 7 allocs/op after — roughly 9KB and 53 allocations of library overhead removed per call. Wall clock is unchanged; the saving is allocation and the GC pressure that follows it.
The compiled-regex cache was bounded by DefaultCacheSize (6144), far below the number of distinct patterns a caller can reference in a single pass. At that size entries are evicted before they are reused, so the cache never hits and every call recompiles anyway — marginally worse than no cache. Add a dedicated DefaultRegexCacheSize (200000, a generous multiple of the largest working sets we have measured) so distinct patterns compile once and are reused. It is a ceiling, not a preallocation: memory scales with the patterns actually compiled, and the value is exported so a caller can size it to its own use. Left as the default (Simple) eviction, consistent with resultCache.
`go test -bench . -count=N` calls the benchmark function N times in one process, and `resultCache` is package-level. A counter starting at zero each invocation therefore replayed the same subjects, so from the second run onward every call was served by the result cache: 59 allocs/op on the first run, 5 on the rest, with or without a compiled-pattern cache. That made the benchmark report the cost of a cache hit rather than of compiling, and show no difference between a cached and an uncached implementation — the opposite of what it exists to measure, and visible only if someone ran it the usual way for benchstat. Numbering subjects across the process keeps every call a miss. Baseline now holds 59-60 allocs/op across all runs instead of collapsing to 5.
7d23ed3 to
402d59e
Compare
|
Rebased onto current No checks have run on this PR yet, which I think means a fork PR needs a What the change does. The benchmark was wrong, and I have corrected it. With the corrected benchmark (
So this is an allocation fix, not a latency fix: allocations drop ~88% and Happy to adjust the bound, the cache choice, or the benchmark shape if you would |
1 participant
Closes #322.
What
regex,regex_all,regex_anyandreplace_regexcallregexp.Compileon every invocation. The pattern is normally a constant in the expression while the subject changes between evaluations, so the same pattern is compiled repeatedly — and compilation is the expensive part of these helpers, depending only on the first argument.This adds a
compileRegexhelper backed by a cache keyed on the pattern, and uses it at the four call sites (dsl.go:283,:774,:788,:810).Why the existing result cache does not cover it
dslFunction.Execcaches results for cacheable functions, but the key hashes the function name and every argument:Whenever the subject differs between calls — the common case for an expression evaluated against changing input — the key differs, the cache misses, and the pattern is compiled again. The caching is at call granularity; the reusable work is at pattern granularity.
Design notes
DefaultRegexCacheSizebound (see Cache sizing below); patterns assembled at runtime cannot grow it without limit.*regexp.Regexpis safe for concurrent use, so one compilation can serve every caller.resultCache.Cache sizing
The first revision bounded the cache by the shared
DefaultCacheSize(6144). That is far below the distinct-pattern working set, so it does not actually help the case that motivated the change: the public nuclei-templates HTTP corpus assembles ~53k distinct patterns, with two WordPress fingerprint templates accounting for most of them (wordpress-plugin-detect.yaml: 55,112 calls / 45,748 distinct;wordpress-theme-detect.yaml: 7,521). With 6144 slots against ~53k patterns every entry is evicted before it is reused — the cache never hits and only adds bookkeeping (marginally worse than no cache).Measured by scanning a local server with those two templates, varying the number of responses:
Upstream allocation scales with the number of responses; a correctly sized cache stays flat because each pattern is compiled once and reused thereafter.
This revision adds a dedicated, exported
DefaultRegexCacheSize(200000) — a generous (~4×) multiple of that ~53k working set:Benchmark
Included as
regex_bench_test.go: a constant pattern against a unique ~19 KB subject per iteration, so the result cache cannot hit.About 9 KB and 53 allocations of library overhead removed per call; the ~19 KB remaining is the benchmark constructing its own subject. Wall clock is unchanged — the saving is allocation and the GC pressure that follows from it, not latency.
Happy to drop the benchmark from the PR if you would rather it lived elsewhere.
Verification
go test -race ./...andgolangci-lint run ./...both pass unchanged.Context
Found while profiling Nuclei, where this single line accounted for 259 GB — 27.5% — of everything one long-running process allocated over 14 hours. Reconfirmed on a current build: ~280 GB, ~26% of all allocation over a ~19h scan. Details and the profile output are in #322.