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

Status: RESOLVED FIXED

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

Comment 1

[Daniel Holbert [:dholbert]]

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)

Comment 2

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

Exactly.
Maybe's dtor does only an if check when not initialized.

Comment 3

[Daniel Holbert [:dholbert]]

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 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1368326: Don't bother calling Clear() in destructor for empty nsTArrays.

Review commit: https://reviewboard.mozilla.org/r/145006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145006/

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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...)

Comment 7

[Nathan Froyd [:froydnj]]

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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/627055e30ca4
Don't bother calling Clear() in destructor for empty nsTArrays. r=froydnj

Comment 9

[Daniel Holbert [:dholbert]]

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.)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/627055e30ca4

Comment 11

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

Seems to be fine.
And Gecko is quite a bit faster than Blink in bug 944127.