-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
Heads up! This PR modifies the following files:
|
r? @bholley |
@bors-servo try |
style: Switch to FxHash for the style system. This improves stylist rebuild times by a pretty neat amount, see bug 1386443.
(also, @bzbarsky may want to review) |
💔 Test failed - linux-dev |
MozReview-Commit-ID: Lh0mMmZ2vn9
It looks like the only use of |
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? |
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. |
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 :) |
☔ The latest upstream changes (presumably #17988) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm working on something better than this :) |
This improves stylist rebuild times by a pretty neat amount, see bug 1386443.
This change is