1983159 - Store Host Defined Data in Promise Datastructure Slots, rather than as a single slot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Currently we basically ask the embedding for an object and store it in a slot and use that for 'host defined data'.

If we instead had a couple of different slots, and let embedders put stuff into those slots, we could avoid the overhead of having to allocate a whole JS object to store a pair of values.

I've marked this as blocked on the new JS Micro task queue mostly to avoid merge conflict issues, but we really could do this today.

Comment 1

[Tommy]

I wanted to take a crack at this, and I just wanted to know if I'm approaching it right, I added slots for incumbent global and scheduling state and then instead of passing around host defined data object I pass around both the incumbent global and scheduling state. The reason I ask is that doing it this way seems to mean a lot of small change in different places.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Yep. That’s the general thrust. The idea is to save on object allocation.

I will be excited to see the patches.

Comment 3

[Tommy]

Attachment: Bug 1983159 - Add IncumbentGlobal and optionalHostDefinedData. r=smaug!,arai!

Add IncumbentGlobal and SchedulingState members and fields to later
replace HostDefinedData

Comment 4

[Tommy]

Attachment: Bug 1983159 - Added IncumbentGlobal and optionalHostDefinedData parameters. r=smaug!,arai!

Added IncumbentGlobal and SchedulingState parameters to functions that
take HostDefinedData.

Comment 5

[Tommy]

Attachment: Bug 1983159 - Replaced HostDefinedData with IncumbentGlobal and optionalHostDefinedData. r=smaug!,arai!

Replaced code that deals with HostDefinedData
with IncumbentGlobal and SchedulingState.

Comment 6

[Tommy]

Attachment: Bug 1983159 - Removed HostDefinedData functions and parameter. r=smaug!,arai!

Comment 7

[Tommy]

Sorry about the poor splitting in these patches. There's a few changes I still want to make (I don't think schedulingstate I'm passing around needs to be rooted), but I've been sitting on these changes for a while so wanted to push them.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

I will take a peek today -- though my last day of the year is tomorrow, so there's a good chance we'll have to revisit these in 2026.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

So I've not -completely- reviewed this. I want to also do some local and CI testing too.

But I do want to give some praise: I really appreciate the structured approach you took to this patch sequence. It makes review easier, it makes it more clear how the pieces move, and I think helps clarify the intent.

Well done!

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

OK, so building locally I had some issues with SchedulingStateIsOptimizedOut -- one thing I'd recommend doing is adding --enable-clang-plugin to your mozconfig

In order to test the approach anyhow, I patched your patches as follows:

diff --git a/js/src/builtin/Promise.cpp b/js/src/builtin/Promise.cpp
index b7932864c9..df7f116b0c 100644
--- a/js/src/builtin/Promise.cpp
+++ b/js/src/builtin/Promise.cpp
@@ -50,7 +50,7 @@
 }
 
 constexpr auto IncumbentGlobalIsOptimizedOut = nullptr;
-auto SchedulingStateIsOptimizedOut = JS::PrivateValue(nullptr);
+
 
 enum ResolutionMode { ResolveMode, RejectMode };
 
@@ -2955,7 +2955,7 @@
     if (!cx->compartment()->wrap(cx, &hostDefinedGlobalRepresentative)) {
       return false;
     }
-
+JS::Value SchedulingStateIsOptimizedOut = JS::NullValue();
     Rooted<JS::Value> schedulingStateIsOptimizedOutRooted(cx, SchedulingStateIsOptimizedOut);
     ThenableJob* thenableJob =
         NewThenableJob(cx, ThenableJob::PromiseResolveThenableJob, promise,
@@ -2985,7 +2985,7 @@
   job->setExtendedSlot(ThenableJobSlot_Promise, promiseToResolve);
   job->setExtendedSlot(ThenableJobSlot_Thenable, thenable);
   job->setExtendedSlot(ThenableJobSlot_Handler, ObjectValue(*then));
-
+JS::Value SchedulingStateIsOptimizedOut = JS::NullValue();
   // Step X. HostEnqueuePromiseJob(job.[[Job]], job.[[Realm]]).
   Rooted<JS::Value> schedulingStateIsOptimizedOutRooted(cx, SchedulingStateIsOptimizedOut);
   return cx->runtime()->enqueuePromiseJob(cx, job, promise,

Not a great answer but a quick hack for now :D

So then I get some failures in some WPT tests I know to be relevant; run these as ./mach wpt --headless /test/path (eg ./mach wpt --headless /scheduler/tentative/yield/ to run them all or /scheduler/tentative/yield/yield-cross-origin-propagation.html to run a specific one)

-------------
/scheduler/tentative/yield/yield-cross-origin-propagation.html
  CRASH /scheduler/tentative/yield/yield-cross-origin-propagation.html
/scheduler/tentative/yield/yield-priority-idle-callbacks.html
  CRASH /scheduler/tentative/yield/yield-priority-idle-callbacks.html
/scheduler/tentative/yield/yield-priority-posttask.any.html
  CRASH /scheduler/tentative/yield/yield-priority-posttask.any.html
 2:47.11 INFO Got 3 unexpected results, with 0 unexpected passes

The failures were all

 1:59.70 pid:196221 [Child 196741, DOM Worker] WARNING: 'globalScopeSentinel && globalScopeSentinel->IsAlive()', file /home/matthew/firefox2/dom/workers/RuntimeService.cpp:2385
 1:59.70 pid:196221 [196741] Assertion failure: false (MOZ_ASSERT_UNREACHABLE: WorkerGlobalScope alive after worker shutdown), at /home/matthew/firefox2/dom/workers/RuntimeService.cpp:2386
 1:59.70 pid:196221 #01: mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() (/home/matthew/firefox2/dom/workers/RuntimeService.cpp:2386)
 1:59.70 pid:196221 #02: nsThread::ProcessNextEvent(bool, bool*) (/home/matthew/firefox2/xpcom/threads/nsThread.cpp:1159)
 1:59.70 pid:196221 #03: NS_ProcessNextEvent(nsIThread*, bool) (/home/matthew/firefox2/xpcom/threads/nsThreadUtils.cpp:461)
 1:59.70 pid:196221 #04: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/matthew/firefox2/ipc/glue/MessagePump.cpp:299)
 1:59.70 pid:196221 #05: MessageLoop::RunInternal() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:369)
 1:59.70 pid:196221 #06: MessageLoop::RunHandler() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:362)
 1:59.70 pid:196221 #07: MessageLoop::Run() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:343)
 1:59.70 pid:196221 #08: nsThread::ThreadFunc(void*) (/home/matthew/firefox2/xpcom/threads/nsThread.cpp:375)
 1:59.70 pid:196221 #09: _pt_root (/home/matthew/firefox2/nsprpub/pr/src/pthreads/ptthread.c:194)
 1:59.70 pid:196221 #10: set_alt_signal_stack_and_start(PthreadCreateParams*) (/home/matthew/firefox2/mozglue/interposers/pthread_create_interposer.cpp:81)

Which is basically an indicator that something which didn't used to be alive has been kept alive somehow.

Other failures I spotted, running some tests I know are worth exploring:

3:12.35 TEST_START: /js/builtins/weakrefs/holdings-multiple-values.optional.any.html
 3:12.50 INFO Closing window 4e845deb-74d2-4641-acde-6b7050c78ef2
 3:13.15 pid:226966 [Parent 226966, Main Thread] WARNING: NS_ENSURE_TRUE(she && she->mInfo->mSharedState.Get()) failed: file /home/matthew/firefox2/docshell/shistory/SessionHistoryEntry.cpp:1213
 3:14.14 pid:226966 ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpc_kx7des/runtests_leaks_222370_tab_pid228220.log
 3:14.15 pid:226966 ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpc_kx7des/runtests_leaks_222370_tab_pid228226.log
 3:14.20 pid:226966 [228108] Assertion failure: obj, at /home/matthew/firefox2/obj-debug-browser-x86_64-pc-linux-gnu/dist/include/js/HeapAPI.h:964
 3:14.20 pid:226966 #01: JS::ExposeObjectToActiveJS(JSObject*) (/home/matthew/firefox2/obj-debug-browser-x86_64-pc-linux-gnu/dist/include/js/HeapAPI.h:964)
 3:14.20 pid:226966 #02: mozilla::FinalizationRegistryCleanup::DoCleanup() (/home/matthew/firefox2/xpcom/base/CycleCollectedJSContext.cpp:1529)
 3:14.20 pid:226966 #03: mozilla::FinalizationRegistryCleanup::CleanupRunnable::Run() (/home/matthew/firefox2/xpcom/base/CycleCollectedJSContext.cpp:1470)
 3:14.20 pid:226966 #04: mozilla::RunnableTask::Run() (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:705)
 3:14.20 pid:226966 #05: mozilla::TaskController::RunTask(mozilla::Task*) (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:196)
 3:14.20 pid:226966 #06: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:1325)
 3:14.20 pid:226966 #07: mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:1148)
 3:14.20 pid:226966 #08: mozilla::TaskController::ProcessPendingMTTask(bool) (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:641)
 3:14.20 pid:226966 #09: mozilla::TaskController::TaskController()::$_0::operator()() const (/home/matthew/firefox2/xpcom/threads/TaskController.cpp:333)
 3:14.20 pid:226966 #10: mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() (/home/matthew/firefox2/xpcom/threads/nsThreadUtils.h:550)
 3:14.20 pid:226966 #11: nsThread::ProcessNextEvent(bool, bool*) (/home/matthew/firefox2/xpcom/threads/nsThread.cpp:1167)
 3:14.20 pid:226966 #12: NS_ProcessNextEvent(nsIThread*, bool) (/home/matthew/firefox2/xpcom/threads/nsThreadUtils.cpp:461)
 3:14.20 pid:226966 #13: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/matthew/firefox2/ipc/glue/MessagePump.cpp:85)
 3:14.20 pid:226966 #14: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/matthew/firefox2/ipc/glue/MessagePump.cpp:268)
 3:14.20 pid:226966 #15: MessageLoop::RunInternal() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:369)
 3:14.20 pid:226966 #16: MessageLoop::RunHandler() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:362)
 3:14.20 pid:226966 #17: MessageLoop::Run() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:343)
 3:14.20 pid:226966 #18: nsBaseAppShell::Run() (/home/matthew/firefox2/widget/nsBaseAppShell.cpp:154)
 3:14.20 pid:226966 #19: nsAppShell::Run() (/home/matthew/firefox2/widget/gtk/nsAppShell.cpp:555)
 3:14.20 pid:226966 #20: XRE_RunAppShell() (/home/matthew/firefox2/toolkit/xre/nsEmbedFunctions.cpp:656)
 3:14.20 pid:226966 #21: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/matthew/firefox2/ipc/glue/MessagePump.cpp:235)
 3:14.20 pid:226966 #22: MessageLoop::RunInternal() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:369)
 3:14.20 pid:226966 #23: MessageLoop::RunHandler() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:362)
 3:14.20 pid:226966 #24: MessageLoop::Run() (/home/matthew/firefox2/ipc/chromium/src/base/message_loop.cc:343)
 3:14.20 pid:226966 #25: XRE_InitChildProcess(int, char**, XREChildData const*) (/home/matthew/firefox2/toolkit/xre/nsEmbedFunctions.cpp:598)
 3:14.20 pid:226966 #26: mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*) (/home/matthew/firefox2/toolkit/xre/Bootstrap.cpp:60)
 3:14.20 pid:226966 #27: main (/home/matthew/firefox2/browser/app/nsBrowserApp.cpp:465)
 3:14.20 pid:226966 #28: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x2a1ca)
 3:14.20 pid:226966 #29: __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6 + 0x2a28b)
 3:14.20 pid:226966 #30: ??? (/home/matthew/firefox2/obj-debug-browser-x86_64-pc-linux-gnu/dist/bin/firefox + 0x997b9)
 3:14.20 pid:226966 #31: ??? (???:???)
 3:15.31 pid:226966 [Parent 226966, IPC I/O Parent] WARNING: [1.1]: Drop

I'd recommend fixing, and before submitting again, run

1. ./mach wpt /js/builtins
2. ./mach wpt /scheduler/tentative/yield

After we get those working, next step would be I submit the patches to our try server for CI validation while I re-review.

Comment 11

[Tommy]

Thanks for the comments and I'll make some updates so it'll hopefully be ready to review by next year.

Comment 12

[Tommy]

Attachment: fixup! Bug 1983159 - Add IncumbentGlobal and SchedulingState. r=mgaudet

Comment 13

[Tommy]

Attachment: workaround

Comment 14

[Tommy]

So I addressed most of the issues, will probably finish the rest soon.

The reason behind the test failure is that I wasn't releasing the schedulingState.

The issue though is I'd have to release the schedulingState in a finalizer inside Promise.cpp but Promise.cpp has no notion of WebTaskSchedulingState.

So my work around was to write a function in CycleCollectedJSContext.cpp that releases schedulingState and pass the function pointer to another slot of the object (either ThenableJob and PromiseReactionRecord) and then add a finalizer to ThenableJob and PromiseReactionRecord to call the function pointer to release schedulingState.

Was wondering if you had any ideas?

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

So that's one workaround for sure. Another (probably preferred) option would be to expose the finalization routine in Promise.h; I'd use TraceNonGCThingMicroTask as the model

Means you don't have to store an identical slot in every object.

Comment 16

[Tommy]

Made a few small changes (no need to re-review). Will try to fix test failures with the work around you suggested over the weekend.

Comment 17

[Tommy]

For future reference do my patches actually have to compile on their own or could I have just split them up without making sure they worked on their own? First time having to split patches so I'm not sure about the etiquette.

Correction: "no need to re-review" -> "no need to re-review, yet"

Comment 18

[Tommy]

Attachment: Bug 1983159 - Added release for SchedulingState during Finalization. r=mgaudet

Added release for SchedulingState during Finalization of PromiseReactionRecord and NewThenableJob

Comment 19

[Matthew Gaudet (he/him) [:mgaudet]]

So most recent try push has problems;

https://treeherder.mozilla.org/jobs?repo=try&revision=4a6fe5b044b0a08d6f70a0805384bc13a7e7918a&selectedTaskRun=XhZCifgERZ227Mg1QYAYFA.0

Let me know if you need help running any of them.

Comment 20

[Tommy]

Attachment: fixup! WIP: Bug 1983159 - Replaced HostDefinedData with IncumbentGlobal and SchedulingState. r=mgaudet

Comment 21

[Tommy]

Attachment: Bug 1983159 - Remove HostDefinedGlobal, merge functionality with IncumbentGlobal. r=smaug!,arai

Comment 22

[Tommy]

Attachment: Bug 1983159 - Allow FinalizationQueueObject to get the incumbent global without scheduling state r=smaug!,arai!

Implements GetIncumbentGlobal to replace the GetObjectFromHostDefinedData currently. This avoids the need to include schedulingState which is requires a root.

Comment 23

[Tommy]

Attachment: fixup! Bug 1983159 - Remove HostDefinedGlobal, merge functionality with IncumbentGlobal. r=mgaudet

Comment 24

[Matthew Gaudet (he/him) [:mgaudet]]

(I don't love the naming after these patches, but have opened Bug 2025988 to address that separately)

Comment 25

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1983159 - Store incumbent global and optional host defined data in two slots inside the JS engine r?smaug!,arai!

The general thrust of this patch stack is that we go from having

[Promise              ]
|HostDefinedData Slot | --> JSObject [ 0: InbumbentGlobal,
                                       1: SchedulingState] (almost always exists)

to

[Promise                       ]
| IncumbentGlobal Slot         |
| optionalHostDefinedData Slot | --> JSObject [ 0: SchedulingState ] (almost never exists)

The result of this is improved performance in the common case where the
host never provides anything beyond the incumbent global for the JS engine to carry
about.

This is a rollup of D277008, D277009, D277010, D277011, D282284, D284008.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/725606536e84
https://hg.mozilla.org/integration/autoland/rev/203b853223b9
Store incumbent global and optional host defined data in two slots inside the JS engine r=smaug,arai
https://github.com/mozilla-firefox/firefox/commit/f3f0b3b53960
https://hg.mozilla.org/integration/autoland/rev/421d945ec37a
Allow FinalizationQueueObject to get the incumbent global without scheduling state r=arai,smaug
https://github.com/mozilla-firefox/firefox/commit/8dfb19ec7cb9
https://hg.mozilla.org/integration/autoland/rev/bcb8d355d612
apply code formatting via Lando

Comment 27

[Mayank Bansal]

expected 15% improvement on JS3-async-fs

Comment 28

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/203b853223b9
https://hg.mozilla.org/mozilla-central/rev/421d945ec37a
https://hg.mozilla.org/mozilla-central/rev/bcb8d355d612

Comment 29

[Florin Bilt]

(In reply to Sandor Molnar[:smolnar] from comment #28)

https://hg.mozilla.org/mozilla-central/rev/203b853223b9
https://hg.mozilla.org/mozilla-central/rev/421d945ec37a
https://hg.mozilla.org/mozilla-central/rev/bcb8d355d612

Perfherder has detected a devtools performance change from push bcb8d355d612048367feed8c4c2966a9d58e0065.

No action is required from the author; this comment is provided for informational purposes only.

Improvement	Test	Platform	Options	Absolute values [old vs new]
10%	reload-webconsole:parent-process objects-with-stacks	linux2404-64		100.00 -> 90.00
10%	reload-webconsole:parent-process objects-with-stacks	linux2404-64-shippable		100.00 -> 90.00

Need Help or Information?

If you have any questions, please reach out to [e-mail]. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

Comment 30

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #26)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/725606536e84
https://hg.mozilla.org/integration/autoland/rev/203b853223b9
Store incumbent global and optional host defined data in two slots inside
the JS engine r=smaug,arai
https://github.com/mozilla-firefox/firefox/commit/f3f0b3b53960
https://hg.mozilla.org/integration/autoland/rev/421d945ec37a
Allow FinalizationQueueObject to get the incumbent global without scheduling
state r=arai,smaug
https://github.com/mozilla-firefox/firefox/commit/8dfb19ec7cb9
https://hg.mozilla.org/integration/autoland/rev/bcb8d355d612
apply code formatting via Lando

Perfherder has detected a browsertime performance change from push bcb8d355d612048367feed8c4c2966a9d58e0065.

No action is required from the author; this comment is provided for informational purposes only.

Improvements	Test	Platform	Options	Absolute values [old vs new]
26%	jetstream3 doxbee-promise-Geometric (doc)	macosx1500-aarch64-shippable	fission webrender	68.05 score -> 85.72 score
26%	jetstream3 doxbee-promise-Geometric (doc)	linux2404-64-shippable	fission webrender	37.94 score -> 47.72 score
19%	jetstream3 async-fs-Worst (doc)	linux2404-64-shippable	fission webrender	147.54 ms -> 119.87 ms
18%	jetstream3 doxbee-promise-Average (doc)	windows11-64-24h2-shippable	fission webrender	63.62 ms -> 51.91 ms
18%	jetstream3 doxbee-promise-Geometric (doc)	android-hw-a55-14-0-aarch64-shippable	webrender	25.83 score -> 30.39 score
...	...	...	...	...
8%	jetstream3 async-fs-Average (doc)	android-hw-a55-14-0-aarch64-shippable	webrender	203.11 ms -> 186.45 ms

Need Help or Information?

If you have any questions, please reach out to [e-mail]. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.