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

style: Switch to FxHash for the style system. #17946

Closed
wants to merge 1 commit into from

Conversation

emilio
Copy link
Member

@emilio emilio commented Aug 2, 2017

This improves stylist rebuild times by a pretty neat amount, see bug 1386443.


This change is Reviewable

@highfive
Copy link

highfive commented Aug 2, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/Cargo.toml, components/style/gecko/data.rs, components/style/selector_map.rs, components/style/lib.rs, components/style/invalidation/media_queries.rs and 7 more
  • @canaltinova: components/style/Cargo.toml, components/style/gecko/data.rs, components/style/selector_map.rs, components/style/lib.rs, components/style/invalidation/media_queries.rs and 7 more

@highfive
Copy link

highfive commented Aug 2, 2017

warning Warning warning

  • These commits modify style and layout code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 2, 2017
@emilio
Copy link
Member Author

emilio commented Aug 2, 2017

r? @bholley

@highfive highfive assigned bholley and unassigned Manishearth Aug 2, 2017
@emilio
Copy link
Member Author

emilio commented Aug 2, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit fbbf5c0 with merge 8774284...

bors-servo pushed a commit that referenced this pull request Aug 2, 2017
style: Switch to FxHash for the style system.

This improves stylist rebuild times by a pretty neat amount, see bug 1386443.
@emilio
Copy link
Member Author

emilio commented Aug 2, 2017

(also, @bzbarsky may want to review)

@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 2, 2017
MozReview-Commit-ID: Lh0mMmZ2vn9
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 2, 2017
@mbrubeck
Copy link
Contributor

mbrubeck commented Aug 2, 2017

It looks like the only use of fnv in Stylo is now in the selectors crate. Should we convert that too, and reduce the number of crates in the Stylo build?

@bzbarsky
Copy link
Contributor

bzbarsky commented Aug 2, 2017

The patch seems pretty straightforward. My question is where the free lunch comes from.

That is, given the assumption that FnvHash is competently written, if it's this much slower that must be because it's able to do something that FxHash doesn't do. Presumably we don't care about that something, but do we know what it is and why we don't care about it?

@bholley
Copy link
Contributor

bholley commented Aug 2, 2017

Is it just that the hashing algorithm is faster? I thought we mostly didn't use the hashing algorithm, since we have precomputed hashes on nsIAtom. Unless we have significant traffic on non-atom-keyed hashmaps? If so, what are they?

As for selectors, seems like we should either remove or compile out the dependency on FnvHash, since we don't actually use the codepath in stylo (we use precomputed hash). That could be a followup E-Easy bug though.

@emilio
Copy link
Member Author

emilio commented Aug 2, 2017

Is it just that the hashing algorithm is faster? I thought we mostly didn't use the hashing algorithm, since we have precomputed hashes on nsIAtom. Unless we have significant traffic on non-atom-keyed hashmaps? If so, what are they?

We're still hashing that hash, afaict.

And re @bzbarsky's questions, I think I'd rather point to rust-lang/rust#37229, which has already pretty much all the discussion I can imagine :)

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Aug 7, 2017
@emilio
Copy link
Member Author

emilio commented Aug 7, 2017

I'm working on something better than this :)

@emilio emilio closed this Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants