-
Notifications
You must be signed in to change notification settings - Fork 246
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
RobinDict #501
RobinDict #501
Conversation
Some early points since i know this is not done yet.
|
My bad. That block of code was written in Jupyter Notebook. Thanks for
pointing out. I'll take care of it from now on.
…On Tue 14 May, 2019, 3:05 PM Lyndon White, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/robin_dict.jl
<#501 (comment)>
:
> + h.maxprobe = 0
+ h.totalcost = 0
+ h.idxfloor = 0
+ return h
+end
+
+function rh_search(h::RobinDict{K, V}, key::K) where {K, V}
+ sz = length(h.keys)
+ index = hashindex(key, sz)
+ cdibs = 0
+ while true
+ if h.slots[index] == 0x0
+ return -1
+ elseif cdibs > h.dibs[index]
+ return -1
+ elseif h.keys[index] == key
I'm not sure how to configure you editor,
but I can tell you there are definately tabs on this line.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#501?email_source=notifications&email_token=AIC2RRORR54OCWA3YRXKHOTPVKBURA5CNFSM4HMXWKQKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBYRFHAQ#pullrequestreview-237130626>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIC2RRPMT7WNSYXJUBWX5W3PVKBURANCNFSM4HMXWKQA>
.
|
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
==========================================
+ Coverage 89.11% 93.39% +4.27%
==========================================
Files 31 32 +1
Lines 2408 2529 +121
==========================================
+ Hits 2146 2362 +216
+ Misses 262 167 -95
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
==========================================
- Coverage 89.4% 87.91% -1.49%
==========================================
Files 31 32 +1
Lines 2407 2690 +283
==========================================
+ Hits 2152 2365 +213
- Misses 255 325 +70
Continue to review full report at Codecov.
|
Ready for review @oxinabox |
changes to insert fix rh_insert, write rh_search, getindex
Fix erronous h.count
remove un-necessary print statement
minor corrections in code, add _tablesz for code readability mistake in constructor with pairs
minor changes in rh_insert!, write out rh_delete! correct a blunder in rehash! minor change in sizehint! yet another mistake in sizehint!
change in rh_delete! minor changes in rh_delete! and pop! fix a typo
introduce max_lf(for benchmarking purposes), write constructor for iterables
constructors for tuples and iterables updated to cover all test cases remove un-necessary constant declaration indentation changes
FWIW, the growth factor use usually use for AbstractDict is: |
Bump |
Co-Authored-By: Jameson Nash <[email protected]>
Bump |
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.
Looks fine by me. Good work!
function _setindex!(h::RobinDict{K,V}, key::K, v0) where {K, V} | ||
v = convert(V, v0) | ||
sz = length(h.keys) | ||
(h.count > ROBIN_DICT_LOAD_FACTOR * sz) && rehash!(h, sz<<2) |
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.
Always resizing by a factor of 4 looks sketchy (leads to very low load factors and hence memory waste for long-lived dicts). RobinDict should now be much cheaper to rehash than Base dict.
Maybe just copy what Base does? (i.e. use threshold to decide whether to grow by factor 2 or 4)
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 that on purpose as this change showed some improvement in benchmarks . In details described here
index = rh_search(h, key) | ||
|
||
index > 0 && return h.vals[index] | ||
|
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.
In theory, we could use the partial traversal from rh_search
and continue insertion from there, instead of starting from desired_index
again: rh_search
aborts at the first entry that would be relocated on insertion.
Just leaving that here for posterity (can be separate PR to improve perf).
Given @chethega has approved, I am going to merge this. |
I call this
RobinDict
, which implements Robinhood Hashing technique .delete
/erase
operation forRobinDict