-
Notifications
You must be signed in to change notification settings - Fork 5
Khashl addition #3
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
Conversation
Added more comments to khashl Updated unittests to actually work for khashl Added khashl to package.d
|
@charlesgregory How much of khashl is identical to khash? A bit it seems. Would it be sensible to factor out (e.g. the hash functions), or is it more important that we retain source-level homology for future updates? |
dub.json
Outdated
| }, | ||
| { | ||
| "name": "bench", | ||
| "targetType": "executable", |
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 like what you are trying to do here but note the header of the _benchmark file; it is an embedded dub.sdl and can be run directly from cmdline like a script.
It was my mistake to put it under source/dklib. Please instead move it to a tests/ or examples/ (probably better) and document that it can be run from command line like script
It appears that the uint and ulong hashing functions may be different for khash vs khashl. I think a factored-out pool of shared hashing functions is not out of the question. We can use aliases to keep decent source-level homology. Up to you though. |
|
Interesting, didn't notice that. I was also thinking of the _put and _resize functions I am happy to accept the code as is , if you'll move the benchmark to |
Added a build type to dub.json that can be used to test this: dub test --compiler=dmd --build=unittest-inline
Fix: khash with string key not compiling with dmd if inlining enabled (Nathan Sashihara)
|
@charlesgregory ping |
|
@charlesgregory You'll also need to rebase on current master since we updated dubfile |
|
As a side note I have found that the free statements in the dtors of both khash and khashl can cause SEGSEVs so in normal use I usually comment this out. I notice this most when making a hashmap of hashmaps. Edit: I forgot I actually opened an issue for this #2 |
Added more comments to khashl Updated unittests to actually work for khashl Added khashl to package.d
|
Did a rebase but I think I should've just done a merge. Either way I would do a squash merge to get rid of the commit history mess. |
|
Also do you need to make the changes to khasl that @n8sh made to khash? (regarding inlineing, which I accepted, and parameter |
|
I messed up the rebase/merge so we are closing this pull request and opening a new one. |
Added khashl and made some changes to the benchmark and dub file.
Khashl is hash table that performs deletions without tombstones allowing it to have a smaller
memory footprint for some sacrifices in speed. It uses fibonacci hashing. Described here.
attractivechaos says this about khashl:
Deletion speed vs bytes per key
Most notably:
Time, in msec, for n=500,000 operations benchmarked on WSL (Ryzen 1700); ldc2 -release
There are no retrieval times for the cached version as that benchmark uses integer keys and my khashl template prevents that at compile time as there should be no real benefit for integers.
attractivechaos says this about hash-caching: