[JSC] shrinkFootprintNow(KeepCodeInUse) can release the code of RegExps that are not in use (off by default) - #633
Conversation
209144a to
affb84c
Compare
a45f8e3 to
83d0bac
Compare
Preview Builds
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ALWAYS_INLINE void RegExp::compileIfNecessary(VM& vm, Yarr::CharSize charSize, std::optional<StringView> sampleString) | ||
| { | ||
| m_lastUseEpoch = static_cast<uint8_t>(vm.heap.objectSpace().markingVersion()); |
There was a problem hiding this comment.
🟡 (optional) compileIfNecessary[MatchOnly] now write m_lastUseEpoch unconditionally, but both are also reached on a DFG compiler thread via matchConcurrently() (RegExp.cpp:419/497) while the mutator's RegExp::match writes the same non-atomic byte without taking cellLock(): an unsynchronized concurrent write that TSan will flag even with the new option off. Fix: stamp the epoch only from the VM thread — move the store into matchInlineOnce under if constexpr (matchFrom == Yarr::MatchFrom::VMThread) for both overloads, or make m_lastUseEpoch a relaxed std::atomic<uint8_t>. Same pattern at 2 sites (RegExpInlines.h:105, :265).
Extended reasoning...
RegExp::matchConcurrently at RegExp.cpp:411 and :490 runs inside DFGStrengthReductionPhase on a JIT worklist thread, takes cellLock(), checks hasCodeFor/hasMatchOnlyCodeFor, then calls matchInline<CompilerThread> → matchInlineOnce → compileIfNecessary[MatchOnly] at RegExpInlines.h:151/309, which after this change stores to m_lastUseEpoch at lines 105/265 before the hasCodeFor early return. Concurrently the mutator's RegExp::match (RegExp.cpp:406/485) enters matchInlineOnce<VMThread> and performs the same store WITHOUT holding cellLock() (the fast match path is lock-free by design; only compile/deleteCode take the lock). Before this change compileIfNecessary[MatchOnly] was read-only on that shared fast path, so no writable field was touched from both threads unlocked; after it, two threads perform non-atomic stores to the same uint8_t — a C++ data race (UB) that ThreadSanitizer will report on every DFG constant-fold of a RegExp that the mutator is also matching, regardless of releaseIdleRegExpCodeWhenShrinkingFootprint. In practice both threads…
Verification: nit — the race is real, though benign in practice (both writers store the same one-byte value derived from the same VM's marking version, so no observable misbehavior; it is however technically C++ UB and TSan-visible, and it fires regardless of the new option). Compiler-thread writer: RegExp::matchConcurrently at Source/JavaScriptCore/runtime/RegExp.cpp:411/490 takes `Locker locker {… | nit —…
| v(Double, warmUpMarkedBlockIdleTimeout, 10, Normal, "seconds without a MarkedBlock request before the helper thread releases what it is holding and shuts down"_s) \ | ||
| v(Bool, scribbleFreeCells, false, Normal, nullptr) \ | ||
| v(Bool, decommitUnusedMarkedBlockPages, true, Normal, "after sweeping a MarkedBlock, return its interior OS pages that hold no live cell to the OS (only where OS pages are smaller than a MarkedBlock)") \ | ||
| v(Bool, releaseIdleRegExpCodeWhenShrinkingFootprint, false, Normal, "VM::shrinkFootprintNow(KeepCodeInUse) also drops the compiled code of RegExps that have not matched since the last full collection began"_s) \ |
There was a problem hiding this comment.
🟡 nit (optional): the new option is inserted between decommitUnusedMarkedBlockPages and decommitUnusedMarkedBlockPagesAfterEdenCollections, whose description "also do it for blocks swept after an eden collection" is a continuation of the former; in --options output it now reads as a follow-on to the RegExp option. Fix: move releaseIdleRegExpCodeWhenShrinkingFootprint one line down so the two decommitUnusedMarkedBlockPages* entries stay adjacent.
Extended reasoning...
OptionsList.h:298 defines decommitUnusedMarkedBlockPages and :300 defines decommitUnusedMarkedBlockPagesAfterEdenCollections with the description "also do it for blocks swept after an eden collection (mostly young blocks that are refilled straight away)" — a sentence that only makes sense immediately after the first option's description. Line 299 now wedges the unrelated RegExp option between them, so a user reading jsc --options (which prints entries in declaration order) sees the eden-collection description directly beneath the RegExp-code option and will misread which behavior it modifies. On the base branch the pair is contiguous. Cosmetic/documentation only; no runtime effect.
Verification: nit — OptionsList.h:298 defines decommitUnusedMarkedBlockPages ("after sweeping a MarkedBlock, return its interior OS pages..."), :300 defines decommitUnusedMarkedBlockPagesAfterEdenCollections with description "also do it for blocks swept after an eden collection (mostly young blocks that are refilled straight away)" — a continuation that only reads correctly directly after :298. The…
affb84c to
72ea768
Compare
…ps that are not in use After an interactive session of a large bundled CLI application the executable pool holds 5.0 MB of allocated code (6.7 MB of committed pages) two minutes after the last input, and 3.7 MB (4.7 MB) after the deep-idle VM::shrinkFootprintNow(KeepCodeInUse). By owner: Yarr code of 580 RegExps 3.85 MB (6.6 KB each), Baseline 0.56 MB in use + 0.60 MB cached on unlinked code blocks whose CodeBlocks are gone (released by its lease before the shrink), DFG 0.22 MB. The pool itself is fine: MetaAllocator decommits a page when its last allocation dies and only 1.0-1.7 MB of committed pages are partly used. What stays is RegExp code, which KeepCodeInUse never touches (KeepCodeThatNeedsParsing drops all of it). With Options::releaseIdleRegExpCodeWhenShrinkingFootprint (off by default) the KeepCodeInUse shrink calls RegExp::deleteCode() on every RegExp in the RegExpCache that has not matched since the last full collection began. The embedder shrinks after the program has been at rest well beyond its last idle collection, so that means "not during the quiet period"; a RegExp on a timer keeps its code. A RegExp whose code was dropped is in the NotCompiled state, exactly as after VM::deleteAllRegExpCode(), and compiles again from its pattern when it next matches. - In use: RegExp::compileIfNecessary[MatchOnly], which every match that runs the matcher goes through first (one that the minimum-length filter rejects does not count), stores the low byte of the heap's marking version (one per full collection) in the new RegExp::m_lastUseEpoch. A stale byte that happens to equal the current one after 256 full collections only keeps some code for another round. The byte takes one of the three bytes of padding between m_constructionErrorCode and m_numSubpatterns; RegExp stays in the 80-byte size class. - Optimized code: DFG and FTL never call a RegExp's Yarr code by address. RegExpExec/RegExpTest/... call operations that end up in RegExp::match; RegExpTestInline emits a fresh copy of the matcher into the DFG/FTL code (Yarr::jitCompileInlinedTest from SpeculativeJIT::compileRegExpTestInline and FTL::LowerDFGToB3::compileRegExpTestInline), built from the pattern; the get*MatchAddr() accessors of YarrCodeBlock are only used to print. The first-character bitmap that the fast paths in JIT code test belongs to the RegExp, not to its code; m_minimumSize goes back to 0, which sends their length check to the slow path, and a minimum size folded as a constant is a property of the pattern. So no machine code refers to what deleteCode() frees. What the compiler threads do read is the YarrCodeBlock's inline statistics (DFGStrengthReductionPhase, then compileRegExpTestInline asserts getRegExpJITCodeBlockConcurrently()), so the release only happens when VM::numberOfActiveJITPlans() is zero; plans are started by the mutator, i.e. not during the shrink. The KeepCodeThatNeedsParsing shrink gets the same guarantee from Heap::deleteAllCodeBlocks() completing all plans first. - Running matches: shrinkFootprintNow() refuses to run with a VMEntryScope (JS on the stack), and RegExp::match neither re-enters the run loop nor is called by the embedder outside of the API lock, so no Yarr code is on the native stack of this VM when the shrink runs. deleteCode() takes the RegExp's cellLock like every other state change. - RegExpCache: every RegExp is created through RegExpCache::lookupOrCreate() and, unless its pattern has a syntax error (no code then), is in its weak map for as long as it lives; entries whose RegExp is dead are skipped. The small strong cache (recently created patterns) is left alone: it only keeps RegExp cells alive, not their code. The same application, 20 inputs, idle 240 s, 10 more inputs, idle 240 s: executable pool after each shrink 3.67 MB allocated / 4.70 MB committed -> 0.13 / 0.97, RssAnon at rest 3.7-6 MB lower. The second session compiles ~450 RegExps again, and that is not free: user-mode instructions of the whole process, five interleaved pairs, 33.94 G (33.75-34.24) without and 34.91 G (34.46-35.34) with the option, +0.97 G (+2.9 %) for one wake-up. Three RegExps account for most of both the memory and the cost: patterns of 530 KB, 169 KB and 20 KB (machine-generated) compile to 1.97 MB, 0.71 MB and 0.17 MB of code in two variants each; the other 570 RegExps hold 1.0 MB together. Hence off by default.
83d0bac to
26c7020
Compare
|
|
||
| bool RegExp::wasUsedInCurrentFullCollectionCycle(VM& vm) const | ||
| { | ||
| return m_lastUseEpoch == static_cast<uint8_t>(vm.heap.objectSpace().markingVersion()); |
There was a problem hiding this comment.
🟡 (optional) wasUsedInCurrentFullCollectionCycle only sees uses that pass through compileIfNecessary[MatchOnly], but several hot match paths never do: RegExpObject::matchGlobal/operationRegExpMatchFastGlobalString when hasValidAtom(), the atom branch of replaceUsingRegExpSearch, split's SpecificPattern::Newlines branch, and tryTrimSpaces. A RegExp compiled once and thereafter used only via those fast paths is treated as idle and deleteCode() clears its m_atom/m_specificPattern, dropping the very fast path it is on. Fix: stamp m_lastUseEpoch on those fast-path entries too (or in recordMatch), so "matched since the last full collection" covers atom/specific-pattern matches.
Extended reasoning...
/a/g.exec("a")reachesmatchInlineOnce→compileIfNecessaryat RegExpInlines.h:105 stamps the epoch and compiles (hasCode()becomes true). 2) A full collection bumpsmarkingVersion(). 3)"aaa".match(/a/g)entersRegExpObject::matchGlobal(RegExpObject.cpp:183) →hasValidAtom()is true →collectGlobalAtomMatchesruns a pure string search and callsrecordMatch(..., oneCharacterMatch=true)(RegExpObjectInlines.h:469); nothing on that path touchesm_lastUseEpoch. Same for the atom branch in StringPrototypeInlines.h:747, theNewlinessplit fast path at RegExpPrototype.cpp:930,tryTrimSpacesat StringPrototypeInlines.h:1185, andRegExpSubstringGlobalAtomCache::collectMatches. 4)shrinkFootprintNow(KeepCodeInUse)with the option on:wasUsedInCurrentFullCollectionCyclecompares the stale epoch and returns false, sodeleteCode()runs and (becausehasCode()is true) setsm_atom = String()andm_specificPattern = None(RegExp.cpp:516–517). 5) The next"aaa".match(/a/g)findshasValidAtom()false and falls tocollectMatches→RegExp::match…
Verification: nit — the gap in the "in use" heuristic is real, but the option defaults to off and the consequence when on is a needless recompile, not incorrect behavior. The epoch is stamped in exactly two places, both added by this diff: - RegExpInlines.h:105 in compileIfNecessary: m_lastUseEpoch = static_cast<uint8_t>(vm.heap.objectSpace().markingVersion()); - RegExpInlines.h:265 in… | nit — the…
| for (auto& [key, weakHandle] : m_weakCache) { | ||
| RegExp* regExp = weakHandle.get(); | ||
| if (!regExp || regExp->wasUsedInCurrentFullCollectionCycle(vm)) | ||
| continue; | ||
| regExp->deleteCode(); |
There was a problem hiding this comment.
🟣 pre-existing, widened: deleteCode() clears m_atom, but RegExpCachedResult may hold m_oneCharacterMatch = true for that RegExp; reifying RegExp.$&/lastMatch then ASSERTs hasValidAtom() (debug) or does reverseFind(0) and reports the wrong position (release). The global-atom fast paths (collectGlobalAtomMatches, RegExpSubstringGlobalAtomCache) never reach compileIfNecessary, so a RegExp last matched that way since the last full GC is deleted here even though it is the current recorded match. Fix: stamp the epoch on those fast paths, or leave m_atom intact in deleteCode() (a pattern property, like m_firstCharacterBitmap).
Extended reasoning...
Slot direction (bounds/guards/hooks): the diff removes no cap, retry, or termination check and adds nothing inside a commit/finalizer/listener; the only new guard is !numberOfActiveJITPlans() at VM.cpp:1167, which is the same idiom used at Heap.cpp:2540 and, since only the mutator creates plans and it holds the API lock here, is sufficient. Fell back to free choice.
Path: let r = /a/g; r.test("xay") compiles r via matchInlineOnce → compileIfNecessaryMatchOnly (RegExpInlines.h:265 stamps the epoch, then compile()); hasCode() becomes true. fullGC() bumps MarkedSpace::m_markingVersion. "xay".match(r) reaches RegExpObject::matchGlobal → RegExpObject.cpp:183 hasValidAtom() → collectGlobalAtomMatches (RegExpObjectInlines.h:409), which never calls compileIfNecessary[MatchOnly], so m_lastUseEpoch is not updated; it records m_oneCharacterMatch = true in RegExpCachedResult (RegExpObjectInlines.h:469 → RegExpGlobalDataInlines.h:41). The KeepCodeInUse shrink then walks the weak cache; wasUsedInCurrentFullCollectionCycle compares the stale byte to the new marking…
Verification: pre-existing. The mechanism is real and reachable, but the base branch already fails the same way through the sibling call one line above. Chain: - RegExp::deleteCode() clears the atom: Source/JavaScriptCore/runtime/RegExp.cpp:512-516 — if (!hasCode()) return; … m_atom = String();. - RegExpCachedResult::lastResult() depends on it under m_oneCharacterMatch:… | pre-existing — the base…
1 participant
Stacked on #628 (one commit on top of its branch; it hangs off the KeepCodeInUse shrink mode that PR adds). Off by default.
the other two follow-ups.
Options::releaseIdleRegExpCodeWhenShrinkingFootprint, default off: this PR adds themechanism and says why it is not switched on.
Why
After an interactive session of a large bundled CLI application the executable pool holds 5.0 MB of allocated code (6.7 MB
of committed pages) two minutes after the last input, and 3.7 MB (4.7 MB) after the deep-idle
VM::shrinkFootprintNow(KeepCodeInUse)of #628. By owner: Yarr code of 580 RegExps 3.85 MB (6.6 KB each), Baseline 0.56 MBin use + 0.60 MB cached on unlinked code blocks whose CodeBlocks are gone, DFG 0.22 MB. The pool itself is fine
(MetaAllocator decommits a page when its last allocation dies; only 1.0-1.7 MB of committed pages are partly used). What
stays is RegExp code, which
KeepCodeInUsenever touches (KeepCodeThatNeedsParsingdrops all of it).Measured with the option on (20 inputs, idle 240 s, 10 more inputs, idle 240 s):
The second session compiles ~450 RegExps again. Three of them account for most of both the memory and the cost: generated
patterns of 530 KB, 169 KB and 20 KB that compile to 1.97 MB, 0.71 MB and 0.17 MB of code in two variants each; the other
570 RegExps hold 1.0 MB together. +2.9 % instructions for one wake-up is not a trade to make for every embedder, hence off
by default; an application without such outliers gets ~1 MB for close to nothing and can turn it on.
What
With the option on, the
KeepCodeInUseshrink callsRegExp::deleteCode()on every RegExp in the RegExpCache that has notmatched since the last full collection began. The embedder shrinks after the program has been at rest well beyond its last
idle collection, so that means "not during the quiet period"; a RegExp on a timer keeps its code. A RegExp whose code was
dropped is in the
NotCompiledstate, exactly as afterVM::deleteAllRegExpCode(), and compiles again from its patternwhen it next matches.
RegExp::compileIfNecessary[MatchOnly], which every match that runs the matcher goes through first, stores the lowbyte of the heap's marking version (one per full collection) in the new
RegExp::m_lastUseEpoch. One byte store permatch, in every configuration (this is the only cost with the option off). A stale byte that happens to equal the current
one after 256 full collections only keeps some code for another round. A match that the minimum-length filter rejects
before it reaches the matcher does not count as a use, which errs on the side of releasing.
m_constructionErrorCode;sizeof(RegExp)stays 80(checked on release and debug builds of this branch).
RegExpCache::deleteCodeNotUsedInCurrentFullCollectionCycle()walks the weak map underm_lock; entries whose RegExp isdead are skipped. The small strong cache is left alone: it keeps RegExp cells alive, not their code.
Safety
RegExpExec/RegExpTest/... call operations thatend up in
RegExp::match;RegExpTestInlineemits a fresh copy of the matcher into the DFG/FTL code(
Yarr::jitCompileInlinedTest), built from the pattern; theget*MatchAddr()accessors ofYarrCodeBlockare only usedto print. Re-checked on this base, which has the first-character-bitmap and minimum-length fast paths from upstream: the
bitmap is owned by the RegExp (
m_firstCharacterBitmap), not by its code, anddeleteCode()leaves it alone;m_minimumSizeis reset to 0 bydeleteCode(), which sends the run-time check in JIT code to the slow path, and aminimum size folded into JIT code as a constant is a property of the pattern and stays true.
YarrCodeBlock's inline statistics (DFGStrengthReductionPhase, thencompileRegExpTestInlineassertsgetRegExpJITCodeBlockConcurrently()), so the release only happens whenVM::numberOfActiveJITPlans()is zero; plans are started by the mutator, i.e. not during the shrink. TheKeepCodeThatNeedsParsingshrink gets the same guarantee fromHeap::deleteAllCodeBlocks()completing all plans first.shrinkFootprintNow()refuses to run with aVMEntryScope(JS on the stack) or from inside thecollector, and
RegExp::matchneither re-enters the run loop nor is called by the embedder outside of the API lock, so noYarr code is on the native stack of this VM when the shrink runs.
deleteCode()takes the RegExp'scellLocklike everyother state change (
matchConcurrentlytakes it too).Yarr code; a later DFG compile then finds no inline statistics and does not inline until the RegExp has matched through
the runtime once more. A missed optimization after a deep idle, not a correctness matter.
Tests
All runs against jsc shells (release, and debug + ASan with
ASAN_OPTIONS=detect_stack_use_after_return=0) of this branch(
1b7bf3e5a380) and of the base (209144af8f6c), base lists retaken in the same session.New test
JSTests/stress/shrink-footprint-releases-idle-regexp-code.js(JIT and--useJIT=0): 200 RegExps match, a fullcollection, ten of them match again,
$vm.shrinkFootprintWhenIdle(keepCodeThatNeedsParsing, keepCodeInUse); asserts via$vm.codeBlockCensus().regExpsWithCodethat the idle ones lost their code, that the ten kept theirs, and that all of themmatch and have code again afterwards. 2/2 on release, 2/2 on ASan. With the option off it fails ("idle RegExps lost their
code: 200 -> 200"), so it cannot pass vacuously.
JSTests/stress, release, all 5,849 files (197 skipped by their own directives), failures / passes:
int8-repeat-in-then-out-of-bounds.js, flaky)--collectContinuously=1codeblock-aging-execution-count.js, flaky)--releaseIdleRegExpCodeWhenShrinkingFootprint=1--collectContinuously=1JSTests/stress, debug + ASan, every fourth file (1,462; 41 skipped), option on: 15 / 1406, the same 15 as base.
Because the suite only reaches the new code through the few tests that shrink the footprint, one more run (a throwaway
driver, not part of the PR): 334 RegExp / String.prototype.{replace,match,split} stress tests executed with
run()in ONEVM, then full collection +
shrinkFootprintWhenIdle(KeepCodeInUse), all 334 again, shrink again, all 334 a third time;every file must give the same result in each pass. RegExps with code 3,280 -> 0 after each shrink -> 3,281 after the next
pass; 0 differences; release with default tiering, release with eager non-concurrent JIT
(
thresholdForJITAfterWarmUp=10 thresholdForOptimizeAfterWarmUp=20 thresholdForFTLOptimizeAfterWarmUp=50)@@ASANDRIVER@@.Not done
the measurement should be repeated before flipping the default.