Close perf regressions vs v4.8.0: recursion pool + JsString primitive read alloc#2450
Conversation
98eef8d to
9d48bf5
Compare
|
Pushed a fix for the linux-ARM/macOS test failure: The minimal-correct fix is to only special-case |
The pool added by sebastienros#2413 has a single slot per JintFunctionDefinition.State guarded by Interlocked.Exchange. For tight self-recursion (ack/fib/tak), only the topmost frame can ever be cached — every recursive call beyond that allocates anyway, so the 4 atomic ops per call become pure overhead. Detect direct named self-call at AST analysis (SelfCallAstVisitor walks the function body for CallExpression with callee Identifier matching the function's own name) and gate the pool path on the new IsDirectRecursive flag in JintEnvironment.NewFunctionEnvironment and ScriptFunction.Call. SunSpider controlflow-recursive: 63.56 ms -> 62.28 ms on a noisy measurement (separate isolated run measured 60.79 ms; v4.8.0 baseline 58.71 ms). Trade-off: allocations on the affected functions return to fresh-per-call, since the pool is intentionally bypassed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR sebastienros#2412's inline-cache fast path added `baseValue.GetV(...)` for non-ObjectInstance receivers. For JsString primitives this routes through TypeConverter.ToObject, which calls intrinsics.String.Construct(jsString) and allocates a fresh StringInstance per read. Hot path: any `for (var i = 0; i < s.length; i++)` re-reads `s.length` every iteration — the slot the script picks for the loop bound doesn't help. Mirror Engine.TryHandleStringValue's primitive-aware lookup inline: length is intrinsic, everything else lives on String.prototype and accepts the primitive as `this` without wrapping. Dromaeo StringBase64 allocations: ~6.12 MB -> ~3.62 MB across all 4 cases (byte-for-byte match with v4.8.0). SunSpider string-base64: 8.83 MB -> 5.07 MB. Knock-on alloc reductions of 4-14% on crypto-md5/sha1, string-fasta, string-validate-input, date-format-xparb, ObjectRegExp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the pattern in GlobalObject.Properties.cs: after CreateProperties_Generated() returns, _properties is the HybridDictionary wrapper around the generator-built StringDictionarySlim. AddDangerous skips both duplicate-key probing and SetOwnProperty's validation pipeline. For the four prototypes that add known-new keys after the generator runs — StringPrototype (trimLeft/trimRight aliases), DatePrototype (toGMTString alias), RegExpPrototype (exec — hand-registered to keep HasDefaultRegExpExec's reference identity), IntrinsicTypedArrayPrototype (toString alias to Array.prototype.toString) — switch to _properties!.AddDangerous(...) and presize the dict via [JsObject(ExtraCapacity = N)] so the additions don't trigger a resize. Symbol-key additions stay as SetOwnProperty since they go to the separate _symbols dict (DictionarySlim) which has no AddDangerous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9d48bf5 to
9c6ec94
Compare
Summary
Three small regression fixes uncovered by a SunSpider/Dromaeo/Stopwatch sweep of
mainagainst tagv4.8.0.Details
1. Skip
FunctionEnvironmentpool for direct-recursive functionsThe pool added by #2413 is a single slot per
JintFunctionDefinition.Stateguarded byInterlocked.Exchange. For tight self-recursion (ack/fib/tak), only the topmost frame can ever be cached — every recursive call beyond that allocates anyway, so the 4 atomic ops per call become pure overhead.Detect direct named self-call at AST analysis (
SelfCallAstVisitorwalks the function body forCallExpressionwith calleeIdentifiermatching the function's own name) and gate the pool path on a newIsDirectRecursiveflag inJintEnvironment.NewFunctionEnvironmentandScriptFunction.Call.SunSpider
controlflow-recursive: 63.56 ms → 62.28 ms (separate isolated run measured 60.79 ms; v4.8.0 baseline 58.71 ms). Trade-off: allocations on direct-recursive functions return to fresh-per-call, since the pool is intentionally bypassed.2. Avoid
StringInstancealloc per primitive property readPR #2412's inline-cache fast path added
baseValue.GetV(...)for non-ObjectInstancereceivers. ForJsStringprimitives this routes throughTypeConverter.ToObject, which callsintrinsics.String.Construct(jsString)and allocates a freshStringInstanceper read. Hot path: anyfor (var i = 0; i < s.length; i++)re-readss.lengthevery iteration.Mirror
Engine.TryHandleStringValue's primitive-aware lookup inline:lengthis intrinsic, everything else lives onString.prototypeand accepts the primitive asthiswithout wrapping.StringBase64(avg of 4 cases)string-base64Knock-on alloc reductions of 4-14% on
crypto-md5/sha1,string-fasta,string-validate-input,date-format-xparb, DromaeoObjectRegExp. Time wins of 4-10% on DromaeoObjectRegExpandObjectString.3.
_properties!.AddDangerous(...)for hand-rolled additions in source-gen prototypesMirrors the pattern already used in
GlobalObject.Properties.cs: afterCreateProperties_Generated(),_propertiesis the HybridDictionary wrapper around the generator-built dict;AddDangerousskips both duplicate-key probing andSetOwnProperty's validation pipeline.Four prototypes converted:
StringPrototype(trimLeft/trimRight aliases),DatePrototype(toGMTString),RegExpPrototype(exec — hand-registered to keepHasDefaultRegExpExec's reference identity),IntrinsicTypedArrayPrototype(toString alias toArray.prototype.toString). Each gets[JsObject(ExtraCapacity = N)]so the additions don't trigger a dict resize. Symbol-key additions stay asSetOwnPropertybecause they go to the separate_symbolsdict.Headline numbers (full BDN run, defaults, Ryzen 9 5950X / .NET 10)
35 of 54 measured cases improved vs
main; no regression on the previously-large wins frommain(string-tagcloud still 10× faster than v4.8.0; Stopwatch alloc still ~46% lower).string-base64timestring-base64allocStringBase64 F,FallocObjectRegExp F,Ttimecontrolflow-recursivetimeLinked issue
Refs the perf comparison surfaced in this PR's commits.
Test plan
dotnet test --configuration Release Jint.Tests/Jint.Tests.csproj— 2842/2889 pass on net472/net10.0dotnet test --configuration Release Jint.Tests.CommonScripts/Jint.Tests.CommonScripts.csproj— 28/28 passmain, no regression on the previously-validatedmainwinsJint.Tests.Test262— not run locally; CI will catch any spec divergenceBreaking change?
No. All changes are internal to interpreter/runtime fast paths; observable behavior is unchanged. The Fix 1 case I considered (parser-side
OnNoderemoval, which would have been a behavior change forFunction.prototype.toString()on non-prepared scripts) was withdrawn after the apparent regression turned out to be measurement variance.🤖 Generated with Claude Code