Closed
Bug 1420509
Opened 7 years ago
Closed 7 years ago
stylo: variable substitution are invoked more times than necessary
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | affected |
People
(Reporter: xidorn, Assigned: emilio)
References
(Blocks 2 open bugs)
Details
Looking at the profiles in bug 1420423 comment 3, and go down with my analysis in bug 1420423 comment 10 and 11, it seems that we are doing variable substitution much more times than necessary.
There are two sources of this:
1. we do variable substitution for all properties in both "early" and "other" categories
2. we do variable substitution one time for each subproperty if the variable is in a shorthand property
See the corresponding code in https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/servo/components/style/properties/properties.mako.rs#3299-3320
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → emilio
Reporter | ||
Comment 1•7 years ago
|
||
So what Gecko does for this is, it does the substitution all together for a block, so each unparsed value is resolved at most once for each element: https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/layout/style/nsRuleNode.cpp#2424-2458
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #1)
> So what Gecko does for this is, it does the substitution all together for a
> block, so each unparsed value is resolved at most once for each element:
> https://searchfox.org/mozilla-central/rev/
> a5d613086ab4d0578510aabe8653e58dc8d7e3e2/layout/style/nsRuleNode.cpp#2424-
> 2458
There's no need to do that... But yeah, what we're doing now is pretty lame :)
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> There's no need to do that... But yeah, what we're doing now is pretty lame
> :)
Well, I mean _probably_ there's no need to do that... It seems like a neat trick if you have tons of elements with the same custom properties. Servo has style sharing for that, so... Let's remove the dumbness for now.
Assignee | ||
Comment 4•7 years ago
|
||
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> https://github.com/servo/servo/pull/19370
This fixes the first issue. Are you going to do something for the second one? From the profile, it seems that shorthand parsing functions are outstanding, especially background.
Reporter | ||
Comment 6•7 years ago
|
||
So I guess one solution to the second issue is that, we store the parsed longhands in a local variable in apply_declarations, and check that list before trying to substitite variables.
Longhand declarations from the same shorthand should generally be together (unless they are set from CSSOM), so it should accelerate common cases where declarations are from parsing declaration block.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #6)
> So I guess one solution to the second issue is that, we store the parsed
> longhands in a local variable in apply_declarations, and check that list
> before trying to substitite variables.
>
> Longhand declarations from the same shorthand should generally be together
> (unless they are set from CSSOM), so it should accelerate common cases where
> declarations are from parsing declaration block.
Right, though I don't have a bright idea to do it cleanly... I'd expect this to be a less pressing issue, so I'll spend some time figuring out how to do it properly over the weekend.
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 8•7 years ago
|
||
I'm seeing some "Conditional jump or move depends on uninitialised value(s) at style::properties::shorthands::background::parse_into" failure with the patch I'm working on. [1]
Given that tests are passing, and I didn't touch that part of code at all, I suspect it is something similar to bug 1394696. jseward, do you think I can simply suppress the warning?
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7909dad076ac2e75c87a969111620692ceba5fd&selectedJob=148027121
Flags: needinfo?(jseward)
Reporter | ||
Comment 9•7 years ago
|
||
Simon's comment in servo/servo#19400 made me wonder again what Gecko does here, since there doesn't seem to be any straightforward way to substitute the whole shorthand and apply them.
Then I had a look at the code I quoted in comment 1, and realized that Gecko doesn't do anything special for shorthand resolving either. It also re-parses shorthand for each of its subproperties. So the second issue in comment 0 is invalid.
I think that means we can close this bug now, given that the PR fixing the first issue has been merged in
https://hg.mozilla.org/mozilla-central/rev/52a15c0179b4
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jseward)
Resolution: --- → FIXED
Reporter | ||
Comment 10•7 years ago
|
||
The parsed shorthand cache may still be a useful optimization, but it's not really a regression from what we had in Gecko.
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
status-firefox-esr52:
--- → unaffected
Comment 11•7 years ago
|
||
Xidorn doesn't think we need to bother uplifting this variable substitution optimization to Beta 58. This performance issue was found with stylo-chrome and we don't know of any websites that are affected.
You need to log in
before you can comment on or make changes to this bug.
Description
•