1420496 - stylo: Stylo spent more time in LoadBindings than the old style system

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

This is an issue recognized from stylo-chrome.

See the profiles in bug 1420423 comment 3, when filtering with LoadBindings, it can be seen that it took Stylo 50ms inside that function in total, while it only took the old style system 12ms.

Emilio said he has had a patch for it, so assigning it to him.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

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

Comment 2

[Emilio Cobos Álvarez [:emilio]]

On Xidorn's modified tpaint:

Before:

178.44000000000005
194.6849999999995
184.14499999999953
201.27499999999964
196.48500000000058
184.42000000000007
181.36999999999898
191.58500000000095
177.40999999999985
166.57500000000073

After:

170.30999999999995
164.7149999999997
168.28499999999985
148.6800000000003
154.0699999999997
160.40500000000065
166.78499999999985
149.46999999999935
163.52000000000044
159.10000000000036

Gecko:

132.6899999999996
141.35500000000002
143.10499999999956
145.41499999999996
144.02000000000044
143.84999999999945
126.01499999999942
136.65999999999985
134.6299999999992
146.9449999999997

So still more improvements needed, but definitely an improvement.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1420496: Avoid unnecessary work in SetXBLInsertionPoint.

Now we clear the data before-hand there's no need to do it in SetInsertionPoint.

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

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

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8931804 [details]
Bug 1420496: Avoid unnecessary work in SetXBLInsertionPoint.

Actually this patch is not correct, and the assertions I added about servo data caught this.

I think we should aim for it to be correct, I filed bug 1420546, and bug 1420547 which is, I think, what needs to happen.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

https://reviewboard.mozilla.org/r/202892/#review208266

::: commit-message-e405f:3
(Diff revision 2)
> +We need to get rid of BindingHolder to handle properly the case of an invalid
> +binding URL.

Can you explain this because I don't know what the improper handling is we had before, and why we now use AutoStyleElement later in the function (after the invalid binding related early returns).

::: dom/xbl/nsXBLService.cpp:421
(Diff revision 2)
> +      // If any child was styled, all of them should be styled already, so we
> +      // can bail out.
> +      return;
> +    }
> +
> +    servoSet->StyleNewSubtree(child->AsElement());

childElement

::: dom/xbl/nsXBLService.cpp:426
(Diff revision 2)
> +    servoSet->StyleNewSubtree(child->AsElement());
> +  }
> +}
> +
> +// Ensures that EnsureSubtreeStyled is called on the element on destruction.
> +class MOZ_STACK_CLASS AutoEnsureSubtreeStyled

MOZ_RAII might be better than MOZ_STACK_CLASS (though I see AutoStyleElement below still uses MOZ_STACK_CLASS).

::: dom/xbl/nsXBLService.cpp:493
(Diff revision 2)
> +
> +  // Easy case: The binding was already loaded.
> +  nsXBLBinding* binding = aContent->GetXBLBinding();
> +  if (binding && !binding->MarkedForDeath() &&
> +      binding->PrototypeBinding()->CompareBindingURI(aURL))
> +  {

Nit: "{" on previous line.

::: dom/xbl/nsXBLService.cpp
(Diff revision 2)
> -    // Get the last binding that is in the append layer.
> -    binding->RootBinding()->SetBaseBinding(newBinding);

Why do we no longer do this?

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

https://reviewboard.mozilla.org/r/202892/#review208266

> Can you explain this because I don't know what the improper handling is we had before, and why we now use AutoStyleElement later in the function (after the invalid binding related early returns).

Oh, I guess AutoEnsureSubtreeStyled handles those cases now.  Still, would like an explanation of the BindingHolder stuff.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

https://reviewboard.mozilla.org/r/202892/#review208266

> Oh, I guess AutoEnsureSubtreeStyled handles those cases now.  Still, would like an explanation of the BindingHolder stuff.

Right, so the BindingHolder stuff basically avoided the call to Loadbindings, and that'd mean that rust would leave the subtree unstyled. I found cleaner to just nuke BindingHolder and handle it in LoadBindings, where EnsureSubtreeStyled takes care of it, than doing it in an `else if` condition in every call to `LoadBindings`, which is what we had before.

> MOZ_RAII might be better than MOZ_STACK_CLASS (though I see AutoStyleElement below still uses MOZ_STACK_CLASS).

Will change both.

> Nit: "{" on previous line.

Will do.

> Why do we no longer do this?

This code was dead, `binding` was always null when leaving the previous `if (binding)` block, so I just removed it.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

https://reviewboard.mozilla.org/r/202892/#review208290

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8931770 [details]
Bug 1420496: Bring back the optimization to avoid traversing XBL subtrees that will likely change.

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

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Servo bits @ https://github.com/servo/servo/pull/19380, though the sync service is broken right now :(

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e68fa6e3773d
Bring back the optimization to avoid traversing XBL subtrees that will likely change. r=heycam

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e68fa6e3773d

Comment 14

[Chris Peterson [:cpeterson]]

Xidorn says this is a XBL fix for stylo-chrome. We don't need to uplift to Beta 58 because stylo-chrome is not enabled in 58.