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

Ninja command hashing could use the newer MurmurHash3 function #636

Open
rgeary1 opened this issue Aug 14, 2013 · 8 comments
Open

Ninja command hashing could use the newer MurmurHash3 function #636

rgeary1 opened this issue Aug 14, 2013 · 8 comments

Comments

@rgeary1
Copy link
Contributor

rgeary1 commented Aug 14, 2013

Ninja computes the command line hash with MurmurHash2 (from the comments). There is a newer, faster MurmurHash algorithm, MurmurHash3
https://code.google.com/p/smhasher/wiki/MurmurHash3

Based on the quoted performance stats, MurmurHash3_x64_128 should take 66% of the time of MurmurHash2. Also MurmurHash2 had a flaw in the algorithm.

@nico
Copy link
Collaborator

nico commented Aug 14, 2013

Do you have benchmarks showing how much faster it is? I believe it's at least significantly longer, which is a minus.

The flaw was that it's possible to engineer collisions as far as I know, which isn't a problem for ninja's use case.

@rgeary1
Copy link
Contributor Author

rgeary1 commented Aug 14, 2013

I don't have any numbers other than the performance quoted in the link (which is actually from an old version of the algorithm). I can implement it, shouldn't be hard.

@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Feb 5, 2020

I wouldn't be able to review it, sry.

@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Feb 5, 2020

If the label was "we won't work on this", I would add it. But "help wanted" sounds like "if you write a PR we will be happy to review and merge it" to me. But I wouldn't have the time to review a PR for this, so I don't want to give people wrong impressions.

@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Feb 5, 2020

Closing would be the wrong message IMHO. Tags like "low priority" or "backlog" might be fitting.

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

No branches or pull requests

4 participants