1422653 - stylo: We spend a ridiculous amount of time in GetFlattenedTreeParentNodeInternal / StyleChildrenIterator on stylo-chrome.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

We need to manage to either make them faster, or call them less... I'd hope the first :)

Comment 1

[Chris Peterson [:cpeterson]]

Emilio says this will also be an issue for Shadow DOM.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1422653: Compute whether XBL is involved in ChildIterator lazily.

The only thing that cares about it is the frame constructor, it's a shame that
everyone else (in particular, the style system) needs to pay this extra walk
over the DOM.

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

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8953846 [details]
Bug 1422653: Compute whether XBL is involved in ChildIterator lazily.

https://reviewboard.mozilla.org/r/223036/#review229750

::: dom/base/ChildIterator.h:155
(Diff revision 1)
>      , mXBLInvolved(aOther.mXBLInvolved)
>      , mOriginalContent(aOther.mOriginalContent)
>    {}
>  
> -  bool XBLInvolved() { return mXBLInvolved; }
> +  bool XBLInvolved() {
> +    if (!mXBLInvolved) {

nit: I'd prefer s/!mXBLInvolved/mXBLInvolved.isNothing()/ for clarity.

::: dom/base/ChildIterator.h:184
(Diff revision 1)
> +  // This is lazily computed when asked for it.
> +  Maybe<bool> mXBLInvolved;
>  
>    const nsIContent* mOriginalContent;
>  };

I dislike Maybe<T> members in general (because they usually double the memory requirement for the member due to alignment) and Maybe<bool> in particular since "if (mSomeBooleanSoundingName)" is *very easily* misread.  It's a footgun IMO.

I guess it's fine here, if we always use the accessor method to test the value.
Can we enforce that by making the member private?
Please move the member last to avoid alignment spill.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/283b2a4cb219
Compute whether XBL is involved in ChildIterator lazily. r=mats

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953846 [details]
Bug 1422653: Compute whether XBL is involved in ChildIterator lazily.

https://reviewboard.mozilla.org/r/223036/#review229750

> nit: I'd prefer s/!mXBLInvolved/mXBLInvolved.isNothing()/ for clarity.

Done.

> I dislike Maybe<T> members in general (because they usually double the memory requirement for the member due to alignment) and Maybe<bool> in particular since "if (mSomeBooleanSoundingName)" is *very easily* misread.  It's a footgun IMO.
> 
> I guess it's fine here, if we always use the accessor method to test the value.
> Can we enforce that by making the member private?
> Please move the member last to avoid alignment spill.

Yeah, fair. `Maybe<bool>` in this case isn't really worse than two bools. I could use bitflags to pack it a bit more but it seemed unnecessary trouble, given this is a stack type anyway.

Made that and the function private before landing. Thanks for the review!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1422653: Make FindSiblingInternal take the iterator by ref to please the static analysis.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8953846 [details]
Bug 1422653: Compute whether XBL is involved in ChildIterator lazily.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223036/diff/1-2/

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9268243e4040
Backed out changeset 283b2a4cb219 for bustage in /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6840:26 on a CLOSED TREE

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8954684 [details]
Bug 1422653: Make FindSiblingInternal take the iterator by ref to please the static analysis.

https://reviewboard.mozilla.org/r/223790/#review229998

Comment 10

[Mozilla Autoland]

hg error in cmd: hg rebase -s 84429c176d221ffa1c60b68543cbd71e6b0c9f8f -d 13eacb5892bb: abort: unknown revision '13eacb5892bb'!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df7075a430f4
Make FindSiblingInternal take the iterator by ref to please the static analysis on the next patch. r=mats
https://hg.mozilla.org/integration/autoland/rev/522aa8e25bd6
Compute whether XBL is involved in ChildIterator lazily. r=mats

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/df7075a430f4
https://hg.mozilla.org/mozilla-central/rev/522aa8e25bd6