Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

In the profile I listed in bug 1420423 comment 20 and 21, it can be seen that it still takes more time for stylo to handle LoadBindings.

See https://perfht.ml/2BkLPgd (Gecko) vs https://perfht.ml/2BkKtlD (Stylo).

In the Gecko profile, LoadBindings takes 13.2ms in total, while in the Stylo profile, it takes 24.6ms. (Although it is clearly an improvement over the very early profile in which it takes ~50ms)

I'm not sure if there is anything left to do after bug 1420496.

	Emilio Cobos Álvarez (:emilio)
	Comment 1 • 7 years ago

I can't think of anything particular except maybe trying to avoid the style re-resolve on the binding element... Still I doubt it actually accounts for the 11ms, and it'd be really dirty.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> I can't think of anything particular except maybe trying to avoid the style
> re-resolve on the binding element... Still I doubt it actually accounts for
> the 11ms, and it'd be really dirty.

The style on the binding element needs to be re-resolved in Gecko as well, IIUC, since the stylesheet in bindings can have rule applying to the binding element. So that's probably not something we can optimize here.

	Emilio Cobos Álvarez (:emilio)
	Comment 3 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #2)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> > I can't think of anything particular except maybe trying to avoid the style
> > re-resolve on the binding element... Still I doubt it actually accounts for
> > the 11ms, and it'd be really dirty.
> 
> The style on the binding element needs to be re-resolved in Gecko as well,
> IIUC, since the stylesheet in bindings can have rule applying to the binding
> element. So that's probably not something we can optimize here.

Right, but only when the binding actually has stylesheets. We're resolving it all the time in stylo right now.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 7 years ago

Oh, looking at the profile, it seems to me that in Gecko, LoadBindings doesn't call into style system at all. Maybe it simply generates dirty root and relies on later flush to update them? If so, the comparison may not be really fair here. But can we do something similar in Stylo? Why is it necessary to resolve the style synchronously in Stylo? (Maybe we want to eagerly resolve all the nested bindings somehow?)

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #4)
> Oh, looking at the profile, it seems to me that in Gecko, LoadBindings
> doesn't call into style system at all. Maybe it simply generates dirty root
> and relies on later flush to update them? If so, the comparison may not be
> really fair here. But can we do something similar in Stylo? Why is it
> necessary to resolve the style synchronously in Stylo? (Maybe we want to
> eagerly resolve all the nested bindings somehow?)

Oh, you're right, indeed. Gecko resolves style from nsCSSFrameConstructor directly, while servo only grabs styles from the element.

That's the point of the style system design, that we have a "resolve the styles" phase. XBL bindings are the kind of annoying part because they're created from frame construction, but we still reuse the same mechanism as everywhere else.

	Emilio Cobos Álvarez (:emilio)
	Comment 6 • 7 years ago

Re. resolving the nested bindings... Yeah, that'd be nice, but seems tons of fairly non-trivial code :)

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 7 years ago

(Also, it's not clear you even can, because you can change style when applying a binding... And binding constructors run all sorts of stuff...)

	Chris Peterson [:cpeterson]
	Comment 8 • 7 years ago

Xidorn and Emilio say this bug is invalid because this test is not measuring Gecko's LoadBindings time.