-
Notifications
You must be signed in to change notification settings - Fork 170
Implement optimized linear probing #990
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
1. Create pos_ptr iterator only if linear probing is done writing to a dict
Use key_mask to figure out whether probing is needed for reading a value from the dict.
1. Rename 'linear_probing_*' to 'resolve_collision_*' 2. Implemented 'resolve_collision_*' methods for LLVMDictOptimizedLinearProbing
@certik This is ready for review. Please let me know if anything should be changed here. |
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.
The changes seem fine to me.
So the current benchmarks do not show a slowdown. Is there a benchmark that would show a speedup with this PR? |
"Note - The benefit of this should be visible when we will be using derived data structures as keys (such as tuple, or very long strings). For now the benchmarks don't show any slowdowns." - Quoting from #990 (comment). The reason is quoted below,
|
It might be a "premature optimization" if we can measure the speedup, but it's fine with me to merge, since you believe this will help us in the future. |
Well. The choice is either this or linked lists ( a.k.a separate chaining) we want to avoid key comparison for two keys not having the same hash. Linked lists is not something I would go for as it will add costs with each insertion. The other thing left is what I implemented here. Anyways, I will implement |
I agree to avoid linked lists. I thought we already had a solution in master for when two keys have the same hash. |
Yes we already had collision resolution before this PR. But that compared keys by value at least once. But if you know beforehand that at a certain key hash no collision has happened then we don’t need to compare original keys at all. So while fetching elements from the dict we will get performance benefits for keys for which no collision has happened. For those which have faced collision we already have the algorithm implemented for that and it works well. |
I see, I understand now. Thanks for implementing it. |
Separate chaining in its raw form requires creating linked list for each insertion in the
dict
. This implies expensivemalloc
calls for each insertion. This becomes a major overhead when we scale the number of insertions as can be seen with C++ benchmarks here. However the advantage of separate chaining is that when the length of linked list at a given index is 1 then we know for sure that we don't need to compare keys by value as no collision has happened yet.So, instead of creating LinkedList for each new insertion, I have updated
key_mask
to provide the signal whether linear probing is to be done or not. This way we can have the benefit separate chaining (avoiding comparison of keys by value while reading) and linear probing (cache efficiency, low overhead asmalloc
calls are made only when rehashing the table) both at one time.I am yet to decouple the logic into a separate child class ofLLVMDict
so that we can switch between the two collision resolution strategies easily.Note - The benefit of this should be visible when we will be using derived data structures as keys (such as tuple, or very long strings). For now the benchmarks don't show any slowdowns.