Reduce time spent in nsTArray destructors, when nsAutoMutationBatch goes out of scope

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: smaug, Assigned: dholbert, NeedInfo)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)

Updated

5 months ago
Priority: -- → P1
Whiteboard: [qf]

Updated

5 months ago
Whiteboard: [qf] → [qf:p3]
(Assignee)

Comment 1

5 months 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

5 months ago
Flags: needinfo?(bugs)
Exactly.
Maybe's dtor does only an if check when not initialized.
Flags: needinfo?(bugs)
(Assignee)

Comment 3

5 months 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

5 months ago
I posted a patch to do comment 3; let's see if it passes Try. (Triggered in mozreview UI)
(Assignee)

Comment 6

5 months 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

5 months ago
Attachment #8873618 - Flags: review?(nfroyd)
(Assignee)

Updated

5 months 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

5 months 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+

Comment 8

5 months ago
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

5 months 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

5 months ago
Flags: needinfo?(bugs)

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/627055e30ca4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.