andyferris / Dictionaries.jl

An alternative interface for dictionaries in Julia, for improved productivity and performance
Other
282 stars 28 forks source link

Check hashes before values when getting Indices token #60

Closed jakobnissen closed 3 years ago

jakobnissen commented 3 years ago

Fix #58 Honestly, the gains are pretty modest - like 10% speed improvement. I guess the speed improvement can be arbitrarily high if you cherry pick objects that take a really long time to compare - I picked short strings, as that's probably most representative.

codecov[bot] commented 3 years ago

Codecov Report

Merging #60 (d444944) into master (26d1be0) will increase coverage by 0.06%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   74.00%   74.06%   +0.06%     
==========================================
  Files          19       19              
  Lines        1712     1720       +8     
==========================================
+ Hits         1267     1274       +7     
- Misses        445      446       +1     
Impacted Files Coverage Δ
src/Indices.jl 88.46% <100.00%> (+0.16%) :arrow_up:
src/AbstractIndices.jl 78.31% <0.00%> (-0.22%) :arrow_down:
src/AbstractDictionary.jl 84.18% <0.00%> (+0.06%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 26d1be0...d444944. Read the comment docs.

andyferris commented 3 years ago

I guess the speed improvement can be arbitrarily high if you cherry pick objects that take a really long time to compare - I picked short strings, as that's probably most representative.

I have real-world use cases where the keys are like "$uuid1.$uuid2" and there are lots of identical uuid1 values, so this optimization could be particularly beneficial in that case.

andyferris commented 3 years ago

In the future, we could probably be a bit more clever here and allow Base.isbitsunion(I) as well as I === Symbol

I decided just to add these now.

jakobnissen commented 3 years ago

Yeah, this is much nicer :) LGTM

andyferris commented 3 years ago

OK I published this and #62 in v 0.3.11: https://github.com/JuliaRegistries/General/pull/42936

Thanks again @jakobnissen