1990842 - Wrap Gecko MicroTasks in a JSObject wrapper with finalizer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

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

This was requested as a reliability measure by Olli to avoid ever being in a situation where a MicroTask is leaked.

The general idea is that Gecko MicroTasks will be put into the JS microtask queue wrapped inside a JSObject, which will ensure that if the MicroTask was never processed for any reason, at least when the JSObject finalizer is run we'll ensure that we do ->Release on the pointer decreasing its refcount.

There's a chance this will actually make the story around the SuppressedMicroTask list nicer too.

Comment 1

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

Attachment: WIP: Bug 1990842 - Wrap MicroTaskRunnables in JSObjects with finalizers for safety

Comment 2

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

Attachment: Bug 1990842 - Create JS::UncopyableValue and use as JS::UncopyableMicroTask r?iain!,smaug!

Comment 3

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

Attachment: Bug 1990842 - Enforce copying restrictions in Gecko r?smaug

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/29ea9091ad03
https://hg.mozilla.org/integration/autoland/rev/ba06cca1f77c
Enforce copying restrictions in Gecko r=smaug

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ba06cca1f77c

Comment 6

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

Foolish Matthew.

This turns out to be a huge performance regression for the JS microtask queue, which has turned an expected performance win by enabling the JS micro task queue into a performance loss. See Bug 2001092.

• Profile without this patch: https://share.firefox.dev/3K5DN3x
• Profile with this patch: https://share.firefox.dev/48nfUhf

Yet again, a recurring theme. In an attempt to make things better now the code pays an enormous amount of barrier costs.

My preference would be to revert this patch, but I'm travelling right now so I can't really investigate in too much detail until next week.

In the interim I will get the enablement patch reverted.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)
Creating an object per microtask sounds like the thing we are trying to avoid with moving the queue into the JS engine. Hopefully we can ensure we are leak free without doing this and rely on existing leak checking during testing to catch any problems.