1352532 - Nursery-allocate more DOM nodes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

Olli was thinking it might be worthwhile to nursery-allocate more DOM node wrappers, for eg when JS grabs the text out of a node then drops it. It would just be a matter of tagging them in the IDL, and measuring the effect on nursery stats.

Probably it would be useful to run with JS_GC_PROFILE_NURSERY=100 or similar (i.e., display timing results for any minor GC taking at least 100 microseconds) and compare.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

aha, this isn't quite so simple. Need to fix dom::ReparentWrapper().

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: div_span_text_nursery.diff

depends on bug 1352746

Initial testing shows that lots of objects get collected during minorGC this way.
Don't know yet how this affects to minor GC times.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

the patch from bug 1352746 and from this bug and loading Techcrunch.com for example leads to

#0  0x00007f10a3030c9a in js::gc::Cell::asTenured() (this=0x7f108deb0868) at /home/smaug/mozilla/hg/vsync/js/src/gc/Heap.h:1133
#1  0x00007f10a3796ebb in JSObject::swap(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) (cx=0x7f10921b8000, a=..., b=...) at /home/smaug/mozilla/hg/vsync/js/src/jsobj.cpp:1536
#2  0x00007f10a368a210 in JS_TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) (cx=0x7f10921b8000, origobj=..., target=...) at /home/smaug/mozilla/hg/vsync/js/src/jsapi.cpp:903
#3  0x00007f109daecd3e in xpc::TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) (cx=0x7f10921b8000, origobj=..., target=...)
    at /home/smaug/mozilla/hg/vsync/js/xpconnect/wrappers/WrapperFactory.cpp:667
#4  0x00007f109f744d8f in mozilla::dom::ReparentWrapper(JSContext*, JS::Handle<JSObject*>) (aCx=0x7f10921b8000, aObjArg=...) at /home/smaug/mozilla/hg/vsync/dom/bindings/BindingUtils.cpp:2211
#5  0x00007f109e802ab6 in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JS::Handle<JSObject*>, nsCOMArray<nsINode>&, nsINode*, nsINode**) (
    aNode=0x7f1071140740, aClone=false, aDeep=true, aNewNodeInfoManager=0x7f10741a2680, aReparentScope=..., aNodesWithProperties=..., aParent=0x0, aResult=0x7ffecd149a78)
    at /home/smaug/mozilla/hg/vsync/dom/base/nsNodeUtils.cpp:583
#6  0x00007f109e786af3 in nsNodeUtils::Adopt(nsINode*, nsNodeInfoManager*, JS::Handle<JSObject*>, nsCOMArray<nsINode>&) (aNode=0x7f1071140740, aNewNodeInfoManager=0x7f10741a2680, aReparentScope=..., aNodesWithProperties=...) at /home/smaug/mozilla/hg/vsync/dom/base/nsNodeUtils.h:228
#7  0x00007f109e732808 in nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) (this=0x7f10722c7000, aAdoptedNode=..., rv=...) at /home/smaug/mozilla/hg/vsync/dom/base/nsDocument.cpp:7640
#8  0x00007f109e731e79 in nsDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) (this=0x7f10722c7000, aAdoptedNode=0x7f10711407c0, aResult=0x7ffecd149e88) at /home/smaug/mozilla/hg/vsync/dom/base/nsDocument.cpp:7499
#9  0x00007f109fb2f270 in nsHTMLDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) (this=0x7f10722c7000, source=0x7f10711407c0, _retval=0x7ffecd149e88) at /home/smaug/mozilla/hg/vsync/dom/html/nsHTMLDocument.h:84
#10 0x00007f109e7cb7ec in AdoptNodeIntoOwnerDoc(nsINode*, nsINode*) (aParent=0x7f107106c8b0, aNode=0x7f1071140740) at /home/smaug/mozilla/hg/vsync/dom/base/nsINode.cpp:1532
#11 0x00007f109e7cdfa4 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) (this=0x7f107106c8b0, aReplace=false, aNewChild=0x7f1071140740, aRefChild=0x0, aError=...)
    at /home/smaug/mozilla/hg/vsync/dom/base/nsINode.cpp:2417
#12 0x00007f109e610cc1 in nsINode::InsertBefore(nsINode&, nsINode*, mozilla::ErrorResult&) (this=0x7f107106c8b0, aNode=..., aChild=0x0, aError=...) at /home/smaug/mozilla/hg/vsync/dom/base/nsINode.h:1822
#13 0x00007f109e610d04 in nsINode::AppendChild(nsINode&, mozilla::ErrorResult&) (this=0x7f107106c8b0, aNode=..., aError=...) at /home/smaug/mozilla/hg/vsync/dom/base/nsINode.h:1826
#14 0x00007f109eb8d41f in mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) (cx=0x7f10921b8000, obj=..., self=0x7f107106c8b0, args=...)
    at /home/smaug/mozilla/hg/vsync/ff_build/dom/bindings/NodeBinding.cpp:856


Kannan, since you're more familiar with JS engine, want to take a look if we can make JSObject::swap to deal with objects in nursery?

Comment 4

[Andrew McCreight [:mccr8]]

Can you just trigger a minor GC before doing reparenting? That would probably be easier than making transplanting even more complex, if we can take the perf hit.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

That is one option, but might be rather slow.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

jonco told that AutoDisableGenerationalGC  can be used to trigger minorGC.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

...but initial testing shows that since we trigger minorGC so often, this may actually regress element creation time.

However, this might decrease likelihood for majorGC.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1352532 - Part 2: Allocate more DOM nodes in the nursery r=smaug

Comment 10

[Jon Coppeard (:jonco)]

This now works (without triggering extra minor GC) since bug 1765338 was fixed.

Comment 11

[Jon Coppeard (:jonco)]

Performance results show no real difference: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=3952bd6771eab2e89382c985a65b16e8b206cf81&newProject=try&newRevision=c769518cc00742be6ac7ef12ef8c3ba665bc1e5c&framework=13&page=1

Comment 12

[Jon Coppeard (:jonco)]

Olli, do you think this is worth landing as-is? Are there other performance tests we can do to see whether this is worth it?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Need to understand "// I'm not sure how the wrapper can get cleared now when it didn't before." part in the patch.
But once that is clear, we could try to land early next release cycle? Then we'd have time to look at GC numbers in telemetry before this gets to beta.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: Bug 1352532 - Part 1: Allow wrapper cache to handle changing a nursery wrapper to another nursery wrapper r=smaug

Previously this situation didn't arise and the existing code calls
NurseryWrapperAdded multiple times with the same wrapper in this case. This
causes assertion failures when the nursery wrapper list is processed later on.

Comment 15

[Jon Coppeard (:jonco)]

Thanks for the reviews. I'll land this after the next merge.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddd6bf428327
Part 2: Allocate more DOM nodes in the nursery r=smaug

Comment 17

[Atila Butkovits]

Backed out for landing without part 1.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ae190ae21c2634b7570023c6d34d512e49d7bd2e

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a3be3ef674c
Part 1: Allow wrapper cache to handle changing a nursery wrapper to another nursery wrapper r=smaug
https://hg.mozilla.org/integration/autoland/rev/e2dd95957747
Part 2: Allocate more DOM nodes in the nursery r=smaug

Comment 19

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/1a3be3ef674c
https://hg.mozilla.org/mozilla-central/rev/e2dd95957747

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

The warning "Wrong inner/outer window combination!" in the other bug hints that someone possibly accessing an object from window which has been already closed/unloaded.

Comment 21

[bszekely]

Backed out for causing some regressions.

Backout link

Comment 22

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/afb4b3178b20

Comment 23

[Andrej (:aglavic)]

(In reply to Atila Butkovits from comment #17)

Backed out for landing without part 1.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ae190ae21c2634b7570023c6d34d512e49d7bd2e

== Change summary for alert #34321 (as of Tue, 07 Jun 2022 00:43:47 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
22%	perf_reftest_singletons line-iterator.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	2,752.55 -> 2,156.60
18%	perf_reftest_singletons line-iterator.html	windows10-64-shippable-qr	e10s fission stylo webrender	2,498.44 -> 2,057.37
14%	perf_reftest_singletons line-iterator.html	linux1804-64-shippable-qr	e10s fission stylo webrender	2,392.85 -> 2,048.75
14%	perf_reftest coalesce-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	70.08 -> 60.11
13%	perf_reftest coalesce-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	69.19 -> 60.28
...	...	...	...	...
6%	perf_reftest_singletons coalesce-1.html	windows10-64-shippable-qr	e10s fission stylo webrender	178.73 -> 168.44

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34321

Comment 24

[Jon Coppeard (:jonco)]

This didn't appear to make any difference to telemetry in the couple of days it was in the codeline.

Comment 25

[Jon Coppeard (:jonco)]

The difference in talos results dame from finalizing dead wrapper objects during minor GCs which happen as part of the test, rather than during major GC which is run after the test finishes. So I think these changes can be ignored.

Comment 26

[Jon Coppeard (:jonco)]

Attachment: Bug 1352532 - Part 3: Ensure the GC sees consistent state while adopting DOM nodes r?smaug

The crashes that happened when this bug originally landed were caused by GC
moving newobj before its native object pointer had been set, so the native
wasn't updated to point to the new object location.

To fix this the native pointer is set immediately to keep everything
consistent. However we have to update it again if transplanting doesn't end up
returning newobj.

Comment 27

[Jon Coppeard (:jonco)]

Soft freeze starts in two days so I'll wait until the next cycle to try landing this again.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87523c63f36e
Part 1: Allow wrapper cache to handle changing a nursery wrapper to another nursery wrapper r=smaug
https://hg.mozilla.org/integration/autoland/rev/9515fe1fed8d
Part 2: Allocate more DOM nodes in the nursery r=smaug
https://hg.mozilla.org/integration/autoland/rev/d984d6438269
Part 3: Ensure the GC sees consistent state while adopting DOM nodes r=smaug

Comment 29

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/87523c63f36e
https://hg.mozilla.org/mozilla-central/rev/9515fe1fed8d
https://hg.mozilla.org/mozilla-central/rev/d984d6438269