Closed Bug 1352532 Opened 6 years ago Closed 5 months ago

Nursery-allocate more DOM nodes

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
104 Branch
Tracking Status
firefox104 --- fixed

People

(Reporter: sfink, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

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.
Blocks: 1352533
aha, this isn't quite so simple. Need to fix dom::ReparentWrapper().
Attached patch div_span_text_nursery.diff (obsolete) — Splinter Review
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.
Priority: -- → P2
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?
Flags: needinfo?(kvijayan)
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.
That is one option, but might be rather slow.
jonco told that AutoDisableGenerationalGC  can be used to trigger minorGC.
Attached patch v2 (obsolete) — Splinter Review
...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.
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
Priority: P2 → P3
Component: DOM → DOM: Core & HTML
Flags: needinfo?(kvijayan)
Depends on: 1765338
Attachment #8853701 - Attachment is obsolete: true
Attachment #8877546 - Attachment is obsolete: true

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

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?

Flags: needinfo?(bugs)

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.

Flags: needinfo?(bugs)

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.

Assignee: nobody → jcoppeard
Status: NEW → ASSIGNED
Attachment #9273808 - Attachment description: Bug 1352532 - Allocate more DOM nodes in the nursery → Bug 1352532 - Part 2: Allocate more DOM nodes in the nursery r?smaug

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

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddd6bf428327
Part 2: Allocate more DOM nodes in the nursery r=smaug
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

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.

Backed out for causing some regressions.

Backout link

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 103 Branch → ---
Regressions: 1772553

(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

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

Flags: needinfo?(jcoppeard)

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.

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.

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

Attachment #9277291 - Attachment description: Bug 1352532 - Part 1: Allow wrapper cache to handle changing a nursery wrapper to another nursery wrapper r?smaug → Bug 1352532 - Part 1: Allow wrapper cache to handle changing a nursery wrapper to another nursery wrapper r=smaug
Attachment #9273808 - Attachment description: Bug 1352532 - Part 2: Allocate more DOM nodes in the nursery r?smaug → Bug 1352532 - Part 2: Allocate more DOM nodes in the nursery r=smaug
Pushed by jcoppeard@mozilla.com:
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
Status: REOPENED → RESOLVED
Closed: 6 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch
Regressions: 1778149
You need to log in before you can comment on or make changes to this bug.