-
Notifications
You must be signed in to change notification settings - Fork 28
Use fnv64a instead of sha256 for cache keys #15
base: master
Are you sure you want to change the base?
Conversation
|
Any idea what the impact would be in terms of cryptographic properties? Further down the track I'd like to implement cache peering, which would expose the cache keys to peers that requested them. |
|
Also, mind rebasing master for the test fixes? |
|
I'd be interested to see how MD5 compares, it's what Squid uses for cache peering: http://wiki.squid-cache.org/SquidFaq/CacheDigests |
|
I updated https://gist.github.com/hectorj/8ac959c071d44ec4d718 with MD5 & SHA1, and also added memory information to the results. MD5 performs better than the SHA family, but is also known as being cryptographically insecure : https://en.wikipedia.org/wiki/MD5#Security So if we need security I'd go with one of the SHA, and if we don't (and I don't think we do), FNV hands down
First time I hear about cache peering so I'm not sure, but peers are supposed to be trusted, no ? |
|
Rebased |
|
What about SipHash? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine a part of the speedup comes from using Write method directly, instead of going through io.WriteString which incurs a func call and additional computation (a type assertion which fails).
Have you tried benchmarking with sha256 but rewriting it to do h.Write([]byte(key)) as well? How does that compare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did in there : https://gist.github.com/hectorj/8ac959c071d44ec4d718#file-cache_key_hash_test-go-L19
Though I only included the fastest results in the gist, and I don't have the numbers right here right now (will get them once I'm home)
IIRC Write([]byte(key)) instead of WriteString does help a bit, but FNV stays faster than sha256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC Write([]byte(key)) instead of WriteString does help a bit, but FNV stays faster than sha256
Cool, thanks. I was just curious.
|
I'm fine to merge this, any objections anyone? |
|
I'm doing some planning on a v2.0.0, I think this would be a good point to change the hashing algorithm on keys, as I plan to change the architecture of the backends. |
I've done some benchmarks, and FNV is ~3 times faster than SHA256
https://gist.github.com/hectorj/8ac959c071d44ec4d718
I know it's only ~1 micro second by hashing, but it's also only a 3 lines change.