use fnv instead of SipHasher for HashMaps. add type FnvMap #106
Conversation
| pub type TcType = ast::AstType<Symbol>; | ||
| pub type TcIdent = ast::TcIdent<Symbol>; | ||
|
|
||
| pub type FnvMap<K, V> = HashMap<K, V, BuildHasherDefault<FnvHasher>>; |
There was a problem hiding this comment.
I wonder if we could have a gluon_base::fnv module, with:
extern crate fnv;
...
pub use self::fnv::FnvHasher;
pub type FnvMap<K, V> = HashMap<K, V, BuildHasherDefault<FnvHasher>>;
|
Sure, I'll move it out of |
|
Just so you understand my reasoning, the name |
|
Yep, I follow, I appreciate your guidance, thanks! I've run the tests again following the refactor, everything seems fine. |
| use std::fmt; | ||
| use std::hash::Hash; | ||
| use std::iter::{FromIterator, IntoIterator}; | ||
| use std::ops::{Index, IndexMut}; | ||
|
|
||
| use fnv::FnvMap; |
There was a problem hiding this comment.
Nit: Could you add a space between this and the doc comment?
|
In your PR description, can you add 'Fixes #95' at the top? That will mean that the issue will be auto-closed when we merge. You also might be interested in joining our gitter channel (if you didn't know about it already) where we have more real-time discussions: https://gitter.im/Marwes/gluon |
👍 Otherwise it is also possible to add this in the merge commit. |
| /// | ||
| /// The default hashing implementation in std::collections uses `SipHasher` | ||
| /// since gluon doesn't need the cryptographic quarantee provided by SipHasher, | ||
| /// we've opted for the faster fnv hash. |
There was a problem hiding this comment.
Should be above the struct! ^_^
|
The results vary quite a lot but for typechecking I am seeing a 5-10% speedup. Probably a bit overshadowed by inefficiencies elsewhere but its still a nice speedup! Before After (clone_prelude measure overhead due to cloning the AST every iteration in |
|
I seem to have caused caused some merge conflicts by merging #102, sorry! |
|
Is there anything you still want me to do on this? Do you want me to rebase or merge and resolve the conflicts? |
|
Just rebase and move the comment as @brendanzab wrote and this should be good! |
|
Merged! Thanks! |
|
Thanks again @leshow! |
@brendanzab mentioned putting the FnvMap alias elsewhere, the map appears to be most used in base/src though. I'm open to any suggestions.
All tests pass.
Fixes #95