Closed Bug 1828496 Opened 5 months ago Closed 5 months ago

Broaden MegamorphicStoreSlot's domain to avoid SetPropertyCache

Categories

(Core :: JavaScript Engine, task, P1)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
115 Branch
Tracking Flags:
Tracking Status
firefox115 --- fixed

People

(Reporter: dthayer, Assigned: dthayer)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [sp3])

Attachments

(10 files)

Bug 1828496 - Replace PropertyName* with PropertyKey for megamorphic accessor ops r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Allow baseline MegamorphicSetElement to be a little faster off x86 r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Don't bother with receiver param for SetElementMegamorphic r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Broaden the scope of MegamorphicStoreSlot r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Use cache from MegamorphicStoreSlot r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Merge SetElementMegamorphic(Cached) r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Ensure we don't add watched objs to megamorphic cache r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Relax maybeTransition's numFailures logic r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Grow MegamorphicSetPropCache r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1828496 - Dynamically allocate MegamorphicSetPropCache r?iain
48 bytes, text/x-phabricator-request
Details | Review

MegamorphicStoreSlot is both more conservative and less performant than MegamorphicSetElement, and importantly because of this more conservative nature, we often don't have any megamorphic strategy for certain types of code, and just go to a generic SetPropertyCache. We can rectify this by broadening MegamorphicStoreSlot and tying it into the MegamorphicSetPropCache to ensure that it is as performant as possible.

Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Severity: -- → N/A
Priority: -- → P1
Whiteboard: [sp3]

This is pretty hard to hit. I actually wasn't able to make a cleaner testcase
than the one I edited, which needs JS_GC_ZEAL=IncrementalMultipleSlices to
get hit. In any case, this is an existing correctness issue with megamorphic
SetElem in Ion, and becomes an issue with megamorphic SetProp with the patch
stack under this.

Depends on D176110

Depends on D176430

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8862075d87d9
Replace PropertyName* with PropertyKey for megamorphic accessor ops r=iain
https://hg.mozilla.org/integration/autoland/rev/d55c962e9240
Allow baseline MegamorphicSetElement to be a little faster off x86 r=iain
https://hg.mozilla.org/integration/autoland/rev/2dad91320e75
Don't bother with receiver param for SetElementMegamorphic r=iain
https://hg.mozilla.org/integration/autoland/rev/1c3ddad06df1
Broaden the scope of MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/38d50428a277
Use cache from MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/167edc0ba04c
Merge SetElementMegamorphic(Cached) r=iain
https://hg.mozilla.org/integration/autoland/rev/683f23dfc2e1
Ensure we don't add watched objs to megamorphic cache r=iain
https://hg.mozilla.org/integration/autoland/rev/0fbe7dd6f404
Relax maybeTransition's numFailures logic r=iain
https://hg.mozilla.org/integration/autoland/rev/c93794786b84
Grow MegamorphicSetPropCache r=iain

This bypasses a very confusing issue where someone, somewhere seems to be
assuming that sizeof(JSRuntime) fits in a uint16_t. Or something like that.
Just adding dead space to it to push it over that boundary causes a leak
in a test. In any case, this works us toward something we might want to do
anyway, which is to lazily instantiate the MegamorphicSetPropCache in order
to save memory when it's not used. I didn't do the same change for the
MegamorphicCache (the get-handling one), because it is accessed from C++
even in hot cache hit paths, where obviously we can't bake pointers to it
into the binary, so there's a slightly plausible case that the extra
pointer dereference could matter.

Depends on D176448

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a6d13c9e6d2
Replace PropertyName* with PropertyKey for megamorphic accessor ops r=iain
https://hg.mozilla.org/integration/autoland/rev/5ec1344315a8
Allow baseline MegamorphicSetElement to be a little faster off x86 r=iain
https://hg.mozilla.org/integration/autoland/rev/ff9e9c9054cc
Don't bother with receiver param for SetElementMegamorphic r=iain
https://hg.mozilla.org/integration/autoland/rev/3d940e8bdb54
Broaden the scope of MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/fe7b04fa7511
Use cache from MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/bfe814423962
Merge SetElementMegamorphic(Cached) r=iain
https://hg.mozilla.org/integration/autoland/rev/bb6078e71f86
Ensure we don't add watched objs to megamorphic cache r=iain
https://hg.mozilla.org/integration/autoland/rev/7fa70275b7b1
Relax maybeTransition's numFailures logic r=iain
https://hg.mozilla.org/integration/autoland/rev/02b78967c120
Grow MegamorphicSetPropCache r=iain
https://hg.mozilla.org/integration/autoland/rev/8700c08f79f5
Dynamically allocate MegamorphicSetPropCache r=iain

Backed out for causing crashes on MegamorphicSetPropCache

[task 2023-05-03T21:31:55.360Z] TEST-PASS | testHelperThreadOOM | ok
[task 2023-05-03T21:31:55.360Z] testNewContextOOM
[task 2023-05-03T21:31:55.416Z] Assertion failure: get() (dereferencing a UniquePtr containing nullptr with ->), at /builds/worker/workspace/obj-spider/dist/include/mozilla/UniquePtr.h:280
[task 2023-05-03T21:31:55.416Z] #01: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc6431c]
[task 2023-05-03T21:31:55.416Z] #02: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc4d627]
[task 2023-05-03T21:31:55.416Z] #03: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc53505]
[task 2023-05-03T21:31:55.416Z] #04: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc59961]
[task 2023-05-03T21:31:55.416Z] #05: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc5cd18]
[task 2023-05-03T21:31:55.417Z] #06: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc5de84]
[task 2023-05-03T21:31:55.417Z] #07: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0xc49e3a]
[task 2023-05-03T21:31:55.417Z] #08: JSRuntime::destroyRuntime()[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0x8896c5]
[task 2023-05-03T21:31:55.417Z] #09: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0x7b5ccc]
[task 2023-05-03T21:31:55.417Z] #10: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0x4108ab]
[task 2023-05-03T21:31:55.417Z] #11: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0x49ce96]
[task 2023-05-03T21:31:55.417Z] #12: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x23d0a]
[task 2023-05-03T21:31:55.417Z] #13: ???[/builds/worker/workspace/obj-spider/dist/bin/jsapi-tests +0x235b4a]
[task 2023-05-03T21:31:55.417Z] #14: ??? (???:???)
[task 2023-05-03T21:31:55.417Z] ExceptionHandler::GenerateDump cloned child 15488
[task 2023-05-03T21:31:55.417Z] ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2023-05-03T21:31:55.417Z] ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2023-05-03T21:31:55.545Z] in directory /builds/worker/workspace/obj-spider, running ['/builds/worker/workspace/obj-spider/_virtualenvs/build/bin/python3', '/builds/worker/checkouts/gecko/testing/mozbase/mozcrash/mozcrash/mozcrash.py', '/tmp', '/builds/worker/workspace/obj-spider/dist/crashreporter-symbols']
[task 2023-05-03T21:31:55.649Z] mozcrash INFO | Copy/paste: /builds/worker/fetches/minidump-stackwalk/minidump-stackwalk --symbols-url=https://symbols.mozilla.org/ --cyborg=/tmp/tmprzbsw5bm/7a032cdb-c14d-7474-3ee6-45469c35f700.trace /tmp/7a032cdb-c14d-7474-3ee6-45469c35f700.dmp /builds/worker/workspace/obj-spider/dist/crashreporter-symbols
[task 2023-05-03T21:31:57.479Z] mozcrash INFO | Saved minidump as /builds/worker/artifacts/7a032cdb-c14d-7474-3ee6-45469c35f700.dmp
[task 2023-05-03T21:31:57.479Z] mozcrash checking /tmp for minidumps...
[task 2023-05-03T21:31:57.479Z] PROCESS-CRASH | mozcrash.py | application crashed [@ mozilla::UniquePtr<js::MegamorphicSetPropCache, JS::DeletePolicy<js::MegamorphicSetPropCache> >::operator->]

All right. I think the patch is finally in the clear, but I'm going to wait until after code freeze to land this.

Flags: needinfo?(dothayer)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffa4965c15bb
Replace PropertyName* with PropertyKey for megamorphic accessor ops r=iain
https://hg.mozilla.org/integration/autoland/rev/58b59cdfeba7
Allow baseline MegamorphicSetElement to be a little faster off x86 r=iain
https://hg.mozilla.org/integration/autoland/rev/1d6294070abb
Don't bother with receiver param for SetElementMegamorphic r=iain
https://hg.mozilla.org/integration/autoland/rev/a3d1b7d5886e
Broaden the scope of MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/522430c88cb1
Use cache from MegamorphicStoreSlot r=iain
https://hg.mozilla.org/integration/autoland/rev/dde994d92a84
Merge SetElementMegamorphic(Cached) r=iain
https://hg.mozilla.org/integration/autoland/rev/cc99aea0fa4e
Ensure we don't add watched objs to megamorphic cache r=iain
https://hg.mozilla.org/integration/autoland/rev/908f81fc3d15
Relax maybeTransition's numFailures logic r=iain
https://hg.mozilla.org/integration/autoland/rev/87bdb14719ba
Grow MegamorphicSetPropCache r=iain
https://hg.mozilla.org/integration/autoland/rev/764a0722b38f
Dynamically allocate MegamorphicSetPropCache r=iain
Regressions: 1833078
You need to log in before you can comment on or make changes to this bug.