1844878 - Improve preservation of DOM wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Iain Ireland [:iain]]

To ensure that we don't lose information when assigning a property to a DOM wrapper object and then discarding the wrapper, the code that autogenerates our DOM classes creates an addProperty hook for each class. One significant downside of this approach is that we have to call the hook whenever a property is added, even though every call after the first is redundant. In addition to the overhead of the calls, this also impedes optimization in some cases, because the hook could have arbitrary side-effects.

I've seen this show up in a few places in sp3 perf reports: for example, selection_select in charts-observable-plot, and Ni in React-stockcharts.

One alternative would be to add a new class flag that tells the engine to call a callback whenever the first property is added to an object. This would reduce unnecessary overhead.

Comment 1

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1844878: WIP wrapper hook

This is an extremely rough first draft. It doesn't even compile yet.

Comment 2

[Markus Stange [:mstange]]

I've been trying to estimate the benefit of this optimization but I'm not sure what work it would eliminate.

In this full sp3 profile I found 500 samples in CallAddPropertyHook, which corresponds to 0.27% of sp3 overall. How much of the work in this profile would we expect this fix to eliminate?

I also see CallAddPropertyHook called in this TodoMVC-Angular comparison report in the two functions at position #4 and #6.

Comment 3

[Iain Ireland [:iain]]

It's a little tricky to estimate. This patch eliminates calls to the add property hook after the first one, but the first one is the expensive one anyway. On the other hand, it also lets us use faster paths for subsequent addprops, which is hard to measure without just writing the patch. To some extent, the improvement depends on the fraction of wrapper objects that have more than one property added.

As a rough estimate: the samples in that profile inside nsWrapperCache::PreserveWrapper are mostly still necessary (except for the if (PreservingWrapper()) return; part, which I'm assuming is trivial). Dropping the two implementations of that function leaves 174/500 samples. I would hope for the fix to eliminate a solid chunk of those samples.

The amount of code left to be written is relatively small. Mostly I just need to find time to talk to a DOM person about making the DOM-side code of my prototype less criminal.

Comment 4

[Markus Stange [:mstange]]

Ah I see. Thanks!

Comment 5

[Iain Ireland [:iain]]

My notes say I've seen this problem in Backbone, Preact, WebComponents, Charts-observable, CodeMirror, TipTap, React, ReactRedux, and NewsSite-Next. Not all of them are hot enough to move the needle a lot, but collectively it seems like it would be good to get this finished.

js-perf-next: Get my draft patch working. I think the code inside SpiderMonkey is relatively complete; the missing part is finding a way to rewrite dom::TryPreserveWrapper so that we can call it from inside SM without going through the addProp hook that we're trying to remove.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1844878 - Eliminate redundant addProperty calls for wrapper preservation r=iain!,smaug!

This patch replaces the use of the addProperty hook to preserve DOM wrappers
in favor of calling the wrapper preservation directly based on a class flag.
This additionally allows us to add an object flag on the shape indicating
that objects with that shape have already had their wrappers preserved,
avoiding redundant calls and allowing us to remain in the JIT for objects
with already preserved wrappers.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6ba67f510be
Eliminate redundant addProperty calls for wrapper preservation r=smaug,jandem

Comment 10

[Cristina Horotan [:chorotan]]

Backed out for causing multiple failures

Backout link

Push with failures

A few failure logs:
xpc test_inspectFields.js https://treeherder.mozilla.org/logviewer?job_id=508742430&repo=autoland&lineNumber=3748
mochitest test_bug655297-1.html https://treeherder.mozilla.org/logviewer?job_id=508739329&repo=autoland&lineNumber=2976
bc failures browser_UrlbarInput_searchTerms_searchMode.js https://treeherder.mozilla.org/logviewer?job_id=508743999&repo=autoland&lineNumber=4074
bc failures browser_anti_clickjacking.js https://treeherder.mozilla.org/logviewer?job_id=508739417&repo=autoland&lineNumber=3450

Please check this range to check failure logs

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82c5c8d58fc2
Revert "Bug 1844878 - Eliminate redundant addProperty calls for wrapper preservation r=smaug,jandem" for causing multiple failures

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9487420 [details]
Bug 1844878 - Eliminate redundant addProperty calls for wrapper preservation r=iain!,smaug!

Removing beta uplift request flag, added due to Bug 1844878

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76db8d178062
Eliminate redundant addProperty calls for wrapper preservation r=smaug,jandem

Comment 14

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/76db8d178062