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

Currently we only avoid cascading descendants of a display:none element, while we still cascade all properties of the element itself, which is wasteful.

In Gecko, since properties are computed lazily, a display:none element doesn't need to cascade lots of properties. This makes Stylo do lots of extra work than Gecko.

We may want to change this.

This is the prototype of the proposed change: https://hg.mozilla.org/try/rev/ce63326a43d97203866f0a342b7a8d845fb26738

To complete it, we need to ensure that getComputedStyle would return the correct result.

In addition to this change, we may also want to stop cascading if we find -moz-binding with value other than none in the initial styling. That way we can be saved from an additional restyle on the specific elements.

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

I'd really prefer us not doing that, at least the initial style thingie, since the style resolver doesn't even have that information and it'd be kinda gross to pass it through I fear...

For getComputedStyle to return the proper value we'd need to thread it down from StyleResolverForElement, I guess, which may not be too bad... But still this feels like a hack, I'd really prefer if we can find something better. There's tons of stuff that can be optimized before that :(

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> There's tons of stuff that can be optimized before that :(

I'd like to know what's the "tons of stuff" we can optimize to make it better on chrome styling performance :/ I've really run out of idea.

	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)
> > There's tons of stuff that can be optimized before that :(
> 
> I'd like to know what's the "tons of stuff" we can optimize to make it
> better on chrome styling performance :/ I've really run out of idea.

For starters, lots of the set_foo functions in gecko.mako.rs are very dumb, and do a lot of malloc and cloning and what not unnecessarily.

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

(And that's a fair amount of the cascade_foo functions I'd expect).

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

We also spend about 5% of styling time just on GetFlattenedTreeParent related functions, which is stupid (this is looking at a fresh profile, https://perfht.ml/2jbtdIR).

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> For starters, lots of the set_foo functions in gecko.mako.rs are very dumb,
> and do a lot of malloc and cloning and what not unnecessarily.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> (And that's a fair amount of the cascade_foo functions I'd expect).

I don't expect that. Lots of time under cascade_property is just allocating / initializing style structs, as well as allocating of some necessary objects like URL or Image, as can be seen from https://perfht.ml/2iYJbpu They are there just because we shouldn't be generating them at all (as this bug stands).

There are weird stuff like formatter usage in will-change, but that really doesn't count for a lot of time, and I doubt pulling time into analysing each set_foo function would really come with any observable difference on performance.

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

There is a side effect for this optimization, though, that we would need to create nsStyleDisplay in early category, which can be a wasteful effort if we are able to reuse the reset structs from rule cache.

There have been two properties from nsStyleDisplay in early category, which are animation-name and transition-property, but they are probably not very common to matter a lot. But display is quite common, so there is a chance that we would end up having wasteful allocation for display struct which negatively affects the performance (which may be one of the reason why it doesn't improve the performance as much as I would expect).

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

A solution to this would be that we simply detect the display:none declaration, rather than having it cascaded in early category, then skip the further cascading as far as the declaration is found. In that case, we would need to cascade display if we end up not having rule cache for the current element.

What adds more complexity to this is that, for Gecko, we would also need to record -moz-binding, because it seems that even if an element has display:none, not cascading its binding would still cause issues (for unknown reason, maybe some binding stylesheet overrides display:none from host page on the element?)

That means we need to record display and -moz-binding, and cascade them afterwards.

Maybe still doable, though :/