Skip to content

Replace murmur hash with the fastest hash#545

Closed
blackswanny wants to merge 2 commits intoemotion-js:masterfrom
blackswanny:master
Closed

Replace murmur hash with the fastest hash#545
blackswanny wants to merge 2 commits intoemotion-js:masterfrom
blackswanny:master

Conversation

@blackswanny
Copy link

Here is another option. It is way faster than murmur and even murmur ver 2, which is used in another PR. The source code is at https://github.com/darkskyapp/string-hash
The original work is at http://www.cse.yorku.ca/%7Eoz/hash.html

@blackswanny
Copy link
Author

I have yarn tests failing. Do you use screen snapshots to compare?
Should I run yarn run coverage -- -u? In this case all tests are passed.
I don't know how to debug this kind of tests.

Snapshot Summary
 › 206 snapshot tests failed in 26 test suites. Inspect your code changes or run with `yarn run coverage -- -u` to update them.

Test Suites: 26 failed, 16 passed, 42 total
Tests:       184 failed, 10 skipped, 487 passed, 681 total
Snapshots:   206 failed, 491 passed, 697 total
Time:        77.255s

@tkh44
Copy link
Member

tkh44 commented Jan 23, 2018

You are going to have massive snapshot failures because of the change in hash structure and algorithm. All the stylesheet snapshots will need to be updated.

try this command

jest --watch --no-cache

@tkh44
Copy link
Member

tkh44 commented Jan 23, 2018

Have you tested these 2 hashes side by side with large strings before doing all of this though? The performance difference might not warrant that much work.

@blackswanny
Copy link
Author

I made a test. it's two strings with 10000 chars
According to this one, there is no difference, and they are equal. So it's either i did't test well or results we see for our project do not directly relate to hash itself, but to the way how hash is used and cached , for instance.
https://jsperf.com/csshash/1

@emmatown
Copy link
Member

The benchmarks I showed in #544 had this hash function as string-hash, it was slower than the hashmumur2 function in #544 but faster than the current hashmumur2 function in Chrome. It was slower than both the hashmumur2 functions in Firefox and it was faster than both the hashmumur2 functions in Safari. I think that the hashmumur2 function in #544 is the best choice since it's faster in general and it's the same algorithm so if people have snapshots with hashes, they won't change.

@blackswanny
Copy link
Author

@mitchellhamilton @tkh44 tests seems controversial. We have no exact winner. Once again my results on large strings.
Firefox - looser (2 times)
Chrome - even
Safari - winner (5 times)
We need to investigate deeper issue with hashes. Maybe it's not related with hash itself. However, on glamor in Chrome (as pretty common dev browser) old hash is 4 times slower, whereas according to these tests it should be even

https://jsperf.com/murmur-vs-djb2
image
image
image

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.

3 participants