Closed
Bug 1421305
Opened 7 years ago
Closed 7 years ago
stylo: LoadBindings still takes more time in Stylo than the old style system
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
firefox59 | --- | affected |
People
(Reporter: xidorn, Unassigned)
References
(Blocks 1 open bug)
Details
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.
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.
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.
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.
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?)
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.
Comment 6•7 years ago
|
||
Re. resolving the nested bindings... Yeah, that'd be nice, but seems tons of fairly non-trivial code :)
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...)
Comment 8•7 years ago
|
||
Xidorn and Emilio say this bug is invalid because this test is not measuring Gecko's LoadBindings time.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•