Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cut property cascading if display:none is found #19506

Closed
wants to merge 1 commit into from

Conversation

@upsuper
Copy link
Member

upsuper commented Dec 7, 2017

The display: none optimization mentioned in bug 1422615.


This change is Reviewable

@highfive
Copy link

highfive commented Dec 7, 2017

Heads up! This PR modifies the following files:

  • @bholley: ports/geckolib/glue.rs, components/style/values/specified/mod.rs, components/style/style_resolver.rs, components/style/properties/properties.mako.rs, components/style/traversal.rs and 2 more
  • @canaltinova: components/style/values/specified/mod.rs, components/style/style_resolver.rs, components/style/properties/properties.mako.rs, components/style/traversal.rs, components/style/properties/computed_value_flags.rs and 1 more
  • @emilio: ports/geckolib/glue.rs, components/style/values/specified/mod.rs, components/style/style_resolver.rs, components/style/properties/properties.mako.rs, components/style/traversal.rs and 2 more
@highfive
Copy link

highfive commented Dec 7, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@upsuper
Copy link
Member Author

upsuper commented Dec 7, 2017

r? @emilio

@highfive highfive assigned emilio and unassigned Manishearth Dec 7, 2017
@upsuper
Copy link
Member Author

upsuper commented Dec 7, 2017

@emilio If you still think we probably shouldn't take this... As far as we can find better places to optimize it, I guess I'm fine. Maybe a several percent optimization isn't really worth this relatively complicated change.

@emilio
Copy link
Member

emilio commented Dec 7, 2017

Yeah, I'd prefer to hold this off for a bit until there's no better optimization possible I think... Not sure what @heycam or @SimonSapin think.

@bors-servo
Copy link
Contributor

bors-servo commented Dec 31, 2017

The latest upstream changes (presumably #19662) made this pull request unmergeable. Please resolve the merge conflicts.

@upsuper upsuper force-pushed the upsuper-forks:display-none-opt branch from f744b91 to a374be4 Jan 5, 2018
@upsuper
Copy link
Member Author

upsuper commented Jan 5, 2018

I'd actually like to revive this PR. I think the idea here is straightforward, the mechanism isn't really that complicated, and it does lead to decent performance improvement based on previous measurement.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2018

The latest upstream changes (presumably #19696) made this pull request unmergeable. Please resolve the merge conflicts.

@upsuper
Copy link
Member Author

upsuper commented Jan 9, 2018

So here is a set of new try pushes for this optimization:

Comparisons:

As can be seen from the last comparison, there is a high confidence ~4% improvement on tpaint win64 (as well as several low confidence improvement among the board including ~4% improvement on tpaint win32). And comparing the first and the second comparison, it can be seen that the regression on tpaint win64 is down from ~25% to ~20%, and tpaint win32 from ~28% to ~23%.

I think this is something significant that we should seriously consider taking.

@emilio
Copy link
Member

emilio commented Jan 9, 2018

Another thing I don't love about this patch is that it relies on the optimization to avoid keeping around display: none pseudo styles to be correct, otherwise there are missing checks...

@emilio
Copy link
Member

emilio commented Jan 9, 2018

I still think this is too error-prone and too complex, even if it happens to be correct in its current form.

@bholley
Copy link
Contributor

bholley commented Jan 9, 2018

It seems like xidorn has shown pretty conclusively that this issue is responsible for a sizeable part of our stylo-chrome regression (this is work that gecko doesn't do but stylo does, and eliminating the work fixes about a fifth of the regression). Can you help propose another way to address this issue?

@upsuper
Copy link
Member Author

upsuper commented Jan 9, 2018

We have identified a bigger problem that is bug 1409672. I expect fix to that bug would reduce the improvement we see here since we may not be restyling display:none elements this often after that. We can measure again when that's done.

@upsuper
Copy link
Member Author

upsuper commented Jan 30, 2018

This patch doesn't apply cleanly anymore, and fixing that seems to be non-trivial, so I'm closing this. If we still want to take it, we can do this in a new pr I guess.

@upsuper upsuper closed this Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.