Closed
Bug 1368326
Opened 7 years ago
Closed 7 years ago
Reduce time spent in nsTArray destructors, when nsAutoMutationBatch goes out of scope
Categories
(Core :: DOM: Core & HTML, enhancement, P1)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: smaug, Assigned: dholbert)
References
Details
Attachments
(1 file)
No description provided.
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [qf]
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3]
Assignee | ||
Comment 1•7 years ago
|
||
I want to clarify the plan here -- it wasn't immediately obvious to me what's being suggested here. I *think* you are suggesting that we should use Maybe<AutoTArray<...>> and Maybe<nsTArray<...>> here: AutoTArray<BatchObserver, 2> mObservers; nsTArray<nsCOMPtr<nsIContent> > mRemovedNodes; nsTArray<nsCOMPtr<nsIContent> > mAddedNodes; https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/base/nsDOMMutationObserver.h#765 ...to avoid running ~nsTArray() and ~AutoTArray() in cases where those structs are never used. Is that correct? (With the assumption being that ~nsTArray and ~AutoTArray are slower for an empty array, as compared to ~Maybe)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bugs)
Reporter | ||
Comment 2•7 years ago
|
||
Exactly. Maybe's dtor does only an if check when not initialized.
Flags: needinfo?(bugs)
Assignee | ||
Comment 3•7 years ago
|
||
Per IRC, one alternative might be to add an "!IsEmpty" check around the call to Clear() here:
> ~nsTArray_Impl() { Clear(); }
since Clear seems nontrivial (even for empty arrays) -- it makes a call to RemoveElementsAt (with a length of 0 if we're empty), which calls a few more things.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
I posted a patch to do comment 3; let's see if it passes Try. (Triggered in mozreview UI)
Assignee | ||
Comment 6•7 years ago
|
||
First Try run busted itself due to infra; triggered another one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9797e31d0f9 Also, FWIW: using Maybe<> might be problematic as an optimization here, because: * Apparently GCC generates extra unnecessary code for Maybe<> in the past, so there's a warning against using it on hot paths here: http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/mfbt/Maybe.h#81-83 * Maybe<> for member-vars will throw off the alignment which can ~double the memory required, since it brings along an extra boolean variable. (Not sure if this part matters so much in this case, since this is a stack variable and we probably only ever have one alive at a time...)
Assignee | ||
Updated•7 years ago
|
Attachment #8873618 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Summary: Consider using Maybe with the arrays in nsAutoMutationBatch to make dtor calls faster → Reduce time spent in nsTArray destructors, when nsAutoMutationBatch goes out of scope
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8873618 [details] Bug 1368326: Don't bother calling Clear() in destructor for empty nsTArrays. https://reviewboard.mozilla.org/r/145006/#review149234 I tried looking at other ways to do this, essentially special-casing the `Clear()` call to not require so much work, but it looks like doing so would result in a pile of special-case helpers that would be more complicated than this. So let's use this patch!
Attachment #8873618 -
Flags: review?(nfroyd) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/627055e30ca4 Don't bother calling Clear() in destructor for empty nsTArrays. r=froydnj
Assignee | ||
Comment 9•7 years ago
|
||
Thanks! smaug: after this hits Nightly, would you mind re-profiling the same page/scenario that prompted you to file this bug, to see if this patch helped? (If it doesn't help for some reason, then we might want to back it out for simplicity's sake, and/or dig in deeper to see what's going on.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/627055e30ca4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Reporter | ||
Comment 11•3 years ago
|
||
Seems to be fine.
And Gecko is quite a bit faster than Blink in bug 944127.
Flags: needinfo?(bugs)
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in
before you can comment on or make changes to this bug.
Description
•