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

Change hash implementation #544

Merged
merged 3 commits into from
Jan 25, 2018
Merged

Change hash implementation #544

merged 3 commits into from
Jan 25, 2018

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Jan 20, 2018

What:
Use a different implementation of murmurhash2 https://github.com/garycourt/murmurhash-js/blob/master/murmurhash2_gc.js (react-native-web also uses this)

Why:
Closes #536
It's generally faster and smaller than that the one we use currently but it's the same algorithm so none of the hashes will change.

Chrome results
Chrome results

Firefox results
Firefox results

Safari Results
Safari Results

How:

Checklist:

  • Documentation N/A
  • Tests N/A
  • Code complete

@codecov
Copy link

codecov bot commented Jan 20, 2018

Codecov Report

Merging #544 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/emotion-utils/src/hash.js 100% <100%> (ø) ⬆️

@greggb
Copy link
Member

greggb commented Jan 20, 2018

Perf results on iOS are fairly poor. Any worry about this being a bottleneck?

@emmatown
Copy link
Member Author

@greggb I didn't test these on iOS but I'm guessing you're referring to Safari? While the hashmumur2 functions are slower than string-hash in Safari, they're still faster than all the hash functions are in Chrome so I don't think it'll be a bottleneck and in general it's more likely that userland code will be a bottleneck rather than a hash function.

@greggb
Copy link
Member

greggb commented Jan 24, 2018

@mitchellhamilton That's what I had figured (user code as bottleneck), but I thought it was worth mentioning as older phones may suffer more.

Unitless graph on an X for posterity (couldn't scroll to get numbers)

image uploaded from ios

@emmatown emmatown merged commit d27335d into master Jan 25, 2018
@emmatown emmatown deleted the change-hash-implementation branch January 25, 2018 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hash for css class names is slow
3 participants