[JSC] Freezing built-in prototypes should not permanently disable fast paths - #622
[JSC] Freezing built-in prototypes should not permanently disable fast paths#622Jarred-Sumner wants to merge 1 commit into
Conversation
…t paths Object.freeze on intrinsics (Array.prototype, RegExp.prototype, String.prototype, Promise.prototype, Map/Set.prototype, the Array constructor, Object.prototype) permanently fired JSGlobalObject fast-path watchpoints even though no property value changed. This made str.replace, spread, species creation, hole reads, slice/includes, promise resolution and Object.assign up to 200x slower for the rest of the process. - putDirectInternal: only fire a property's replacement watchpoint when the stored value actually changes, and only when the structure is watching. A defineProperty that only changes attributes no longer invalidates adaptive watchpoints that check the value. - validateAndApplyPropertyDescriptor: reuse the existing GetterSetter when the getter and setter are unchanged, so attribute-only redefinitions keep the same cell and Equivalence conditions on it stay valid. - Object.assign: when only a prototype (not the target) has read-only or accessor properties, check the source keys against the prototype chain instead of always taking the generic path. - Freezing a JSArray with no indexed storage (Array.prototype) keeps blank indexing instead of converting to sparse ArrayStorage. A new Structure bit, didFreeze, makes length read-only. Rejected indexed writes on such arrays throw without converting storage. This keeps the array prototype chain sane.
Preview Builds
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (14)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughThe changes add JavaScriptCore support for frozen dense arrays, prototype-aware ChangesFreeze and assignment behavior
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The reviewed freeze, assignment, and watchpoint changes have no identified actionable merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the bug, implementation changes, tests, and performance results. It omits the required Bugzilla bug title and link, review-status line, and path/function change list from the repository template.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| throw new Error("FAIL: " + msg); | ||
| } | ||
|
|
||
| function warm(f, n = 1e4) { |
There was a problem hiding this comment.
🟡 nit (optional): New stress tests hard-code iteration counts (1e4/1e3) instead of using testLoopCount, which JSTests/README.md (imported by JSTests/CLAUDE.md) lists as a required rule so tests tier up only in configurations where it matters and stay under 200ms elsewhere. sweep:\b1e[34]\b in the three added JSTests/stress files. Fix: drive warm-up loops with testLoopCount (e.g. function warm(f, n = testLoopCount) here; for (let i = 0; i < testLoopCount; i++) in freeze-array-prototype.js:61 and object-assign-frozen-object-prototype.js:21/26/32/42/51). [also at: JSTests/stress/object-assign-frozen-object-prototype.js:20 - nit: new stress tests hard-code 1e4/1e3 iteration counts instead of testLoopCount; JSTests/README.md (imported by…]
Extended reasoning...
JSTests/CLAUDE.md imports JSTests/README.md, whose rule 2 states new tests are required to use testLoopCount/wasmTestLoopCount so the harness can scale iterations per configuration (eager tier-up vs. no-JIT vs. GC-heavy). All three new tests instead hard-code 1e4 (and 1e3) loops: define-property-same-value-keeps-adaptive-watchpoints.js:6/75, freeze-array-prototype.js:27/61/67, object-assign-frozen-object-prototype.js:21/26/32/42/51. In no-JIT or GC-stress configurations these loops run at fixed cost with no tier-up benefit, risking the 200ms budget, and in eager configurations they may over-iterate. Base branch has no such files; the diff introduces the violation.
Verification: nit — JSTests/README.md:20 (imported by JSTests/CLAUDE.md:1 via @ README.md) states as a hard rule for new tests: "Use testLoopCount or wasmTestLoopCount to control how many iterations a test runs. The jsc CLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't." All three new tests hard-code counts…
1 participant
Freezing built-in prototypes (
Object.freezeon Array.prototype, RegExp.prototype, String.prototype, Promise.prototype, Map/Set.prototype,Array, Object.prototype) permanently fired JSGlobalObject fast-path watchpoints even though no values changed, leaving common operations up to 200x slower for the rest of the process. This keeps those fast paths alive after freezing.Changes
putDirectInternal: fire a property's replacement watchpoint only when the structure is watching and the stored value actually changes, so attribute-only redefinitions no longer invalidate value-based adaptive watchpoints.validateAndApplyPropertyDescriptor: reuse the existingGetterSetterwhen getter and setter are unchanged.Object.assign: when only a prototype (not the target) has read-only or accessor properties, check the source keys against the chain instead of always taking the generic path.didFreeze, makeslengthread-only. This keeps the array prototype chain "sane" watchpoint valid.Verification
define-property-same-value-keeps-adaptive-watchpoints.js,object-assign-frozen-object-prototype.js,freeze-array-prototype.js.JSTests/stressfiles, clean HEAD vs an earlier revision of this branch: no unexpected differences (two watchdog/timeout tests flipped between timeout and failure under load; not re-run in isolation). Final revision: ~450-test targeted subset, no unexpected differences.str.replace244→0 ms, hole reads 110→12 ms,slice131→16 ms, speciesmap30→4 ms, promisethenchains 28→12 ms,Object.assign72→41 ms (unfrozen: 29 ms).