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

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

	Xidorn Quan [:xidorn] UTC+11 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

	Emilio Cobos Álvarez (:emilio) 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 :)

	Emilio Cobos Álvarez (:emilio) 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.

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

https://github.com/servo/servo/pull/19370

	Xidorn Quan [:xidorn] UTC+11 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.

	Xidorn Quan [:xidorn] UTC+11 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.

	Emilio Cobos Álvarez (:emilio) 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.

	Xidorn Quan [:xidorn] UTC+11 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

	Xidorn Quan [:xidorn] UTC+11 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

	Xidorn Quan [:xidorn] UTC+11 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.

	Chris Peterson [:cpeterson]
	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.