Faster creation of OrderedDict, revisited #221

phaverty · 2016-10-29T23:07:52Z

This PR enables much faster creation of OrderedDict objects in julia 0.5 and is backwards compatible with 0.4.

…back for infinite iterators is the original loop over k/v pairs

…mpatible with 0.4

codecov-io · 2016-10-29T23:15:34Z

Current coverage is 89.67% (diff: 53.65%)

Merging #221 into master will decrease coverage by 2.54%

@@             master       #221   diff @@
==========================================
  Files            28         29     +1   
  Lines          2338       2363    +25   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           2156       2119    -37   
- Misses          182        244    +62   
  Partials          0          0

Powered by Codecov. Last update 52615e8...2678dd4

…elta to pass for this PR. It is difficult because most of the code is version specific.

…n specific code choice

…on method too, so less code 0.4 only and more test coverage in 0.5

kmsquire · 2016-11-19T17:42:04Z

src/ordered_dict.jl

+next, done, keys, values, setdiff, setdiff!,
+union, union!, intersect, filter, filter!,
+hash, eltype, KeyIterator, ValueIterator, convert, copy,
+merge


It would be nice if this particular spacing change weren't part of this commit.

kmsquire · 2016-11-19T17:42:11Z

test/test_ordered_dict.jl

@@ -6,8 +6,10 @@
 @test isa(OrderedDict(Pair(1, 1.0)), OrderedDict{Int,Float64})
 @test isa(OrderedDict(Pair(1, 1.0), Pair(2, 2.0)), OrderedDict{Int,Float64})
 @test isa(OrderedDict(Pair(1, 1.0), Pair(2, 2.0), Pair(3, 3.0)), OrderedDict{Int,Float64})
+@test isa(OrderedDict([Pair(1, 1.0), Pair(2, 2.0), Pair(3, 3.0)]), OrderedDict{Int,Float64})


Even though Pair was used above, I suspect these tests were written before 1 => 1.0 was equivalent to Pair(1,1.0). I think it would be clearer to change all of these to use => notation.

kmsquire · 2016-11-19T17:50:58Z

src/ordered_dict.jl

+
+    dict_with_eltype{K, V}(kv,  ::Type{Tuple{K, V}}) = OrderedDict{K, V}(kv)
+    dict_with_eltype{K, V}(kv,  ::Type{Pair{K, V}}) = OrderedDict{K, V}(kv)
+    dict_with_eltype(kv,  t) = OrderedDict{Any, Any}(kv)


Nitpick: extra space before t

kmsquire · 2016-11-19T18:03:52Z

src/ordered_dict.jl

+    @static if VERSION >= v"0.5.0"
+        function OrderedDict(kv, isz::Union{HasLength, HasShape})
+            n = length(kv)
+            slotsz = max(16, (n*3)>>1 )


This PR could be a lot smaller if you did something like the following:

@static if VERSION >= v"0.5.0" get_n_slotsz(kv) = get_slot_size(kv, iteratorsize(kv)) get_n_slotsz(kv, isz::SizeUnknown) = (0,16) function get_n_slotsz(kv, isz::Union{HasLength, HasShape}) n = length(kv) slotsz = max(16, (n*3)>>1) return n, slotsz end else get_n_slotsz(kv) = (0,16) end

kmsquire · 2016-11-19T18:05:07Z

src/ordered_dict.jl

+                index = hashindex(k,slotsz)
+                h.keys[i] = k
+                h.vals[i] = v
+                h.slots[index] = i


This won't work, as it doesn't handle index collisions. Use ht_keyindex2.

kmsquire · 2016-11-19T18:07:52Z

src/ordered_dict.jl

+
+    dict_with_eltype{K,V}(kv, ::Type{Tuple{K,V}}) = OrderedDict{K,V}(kv, iteratorsize(kv))
+    dict_with_eltype{K,V}(kv, ::Type{Pair{K,V}}) = OrderedDict{K,V}(kv, iteratorsize(kv))
+    dict_with_eltype(kv, t) = OrderedDict{Any,Any}(kv, iteratorsize(kv))


This block can be removed if you define get_n_slotsz above. (There is probably a better name for that function...)

…t in order to handle hash collisions

phaverty · 2016-11-20T05:09:36Z

Thanks for the thoughtful review @kmsquire . I think I've implemented all of your suggestions.

kmsquire · 2016-11-20T06:25:39Z

src/ordered_dict.jl

+                index = ht_keyindex2(h, k)
+                @inbounds h.keys[i] = k
+                @inbounds h.vals[i] = v
+                @inbounds h.slots[-index] = i


Closer, but there needs to be a little more defensive programming here. Since kv is an arbitrary set of key-value pairs, it's possible that a key is repeated, so you'll need to check the return value of index and respond accordingly. You should double check against setindex! to make sure there aren't any other corner cases.

…ction input

phaverty · 2016-11-21T03:51:19Z

I think this should do it. I spent some time with setindex! and friends. We don't have to worry about converting the types of the incoming keys and values as we have created the new OrderedDict to match those K and V types. ht_keyindex2 should handle any necessary rehashing. This commit handles incoming duplicated keys. I think that's it. Please let me know if you can think of anything else.

kmsquire · 2016-11-21T23:50:24Z

src/ordered_dict.jl

        end
        return h
    end
+


Minor nitpick: spaces at end of line.

phaverty · 2016-11-21T23:58:11Z

I've pushed a commit that cleans up the extra spaces.

Pete

Peter M. Haverty, Ph.D.
Genentech, Inc.
phaverty@gene.com

On Mon, Nov 21, 2016 at 3:52 PM, Kevin Squire notifications@github.com
wrote:

@kmsquire commented on this pull request.

In src/ordered_dict.jl
#221 (review)
:
     end
     return h
 end
Minor nitpick: spaces at end of line.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#221 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AH02K01ojtnbmLrFRm-iDlfrCA9h5-Ceks5rAi7AgaJpZM4KkPgh
.

kmsquire · 2016-11-22T00:01:02Z

Overall, I think this is good.

I'm slightly concerned that OrderedDicts (and this PR) might be susceptible to JuliaLang/julia#15077, but the implementation has changed enough since then that it's hard to test (we pass the test in Base, but the arrays are not sized the same as described here, and I believe if it were sized the same, the test would fail, since we don't have the appropriate checks in place. Cc: @StefanKarpinski

That said, that's no worse than the current situation, and it should be rare. I'll try to test a little later (but if you have time and want to take a poke at it, please do). Either way, I plan to merge this evening (PST) if there are no other comments, and if a fix is needed for the above, we can fix this constructor at the same time.

phaverty · 2016-11-24T02:03:20Z

This new constructor algo works for very short OrderedDicts. Let me think about this one a bit more.

Peter M. Haverty added 2 commits July 27, 2016 21:36

Trait style dispatch on OrderedDict inner constructor now works. Fall…

7f9caa1

…back for infinite iterators is the original loop over k/v pairs

Faster creation of ordered dict now only in julia 0.5 and backward co…

de919ef

…mpatible with 0.4

Peter M. Haverty added 2 commits November 14, 2016 10:55

one more ordered dict test, trying to get OrderedDict test coverage d…

a176da2

…elta to pass for this PR. It is difficult because most of the code is version specific.

grouped version-specific code better, added at static macro to versio…

a585910

…n specific code choice

phaverty closed this Nov 16, 2016

phaverty reopened this Nov 16, 2016

Peter M. Haverty added 3 commits November 16, 2016 15:03

newer compat

33163b1

inner constructor for one or more Pairs can use the faster constructi…

bdc4031

…on method too, so less code 0.4 only and more test coverage in 0.5

do not need the one or more pairs internal constructor

9a7f657

kmsquire requested changes Nov 19, 2016

View reviewed changes

Peter M. Haverty added 7 commits November 19, 2016 11:02

spacing fixes

a8f167b

swapped Pair for => in ordered dict tests

830f402

much simpler way to get ordered dict size information from kmsquire

f6e9769

some inbounds macros like setindex on ordered dict

9211ff4

ht_keyindex2 instead of hashindex in inner constructor for OrderedDic…

2f9cbb2

…t in order to handle hash collisions

actually make 0.4 work

4f52920

put at static back

2e5aa4b

kmsquire requested changes Nov 20, 2016

View reviewed changes

Peter M. Haverty added 3 commits November 20, 2016 10:44

a few stray spaces in the OrderedDict type docstring

32a9ddb

new construction algo, with tests, that handles duplicated keys

49ce624

another way to resize the OD if duplicated keys appear in the constru…

db96306

…ction input

kmsquire reviewed Nov 21, 2016

View reviewed changes

src/ordered_dict.jl

end

return h

end

Copy link

Member

kmsquire Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick: spaces at end of line.

kmsquire approved these changes Nov 22, 2016

View reviewed changes

some blank lines had indenting spaces

a1972c2

kmsquire mentioned this pull request Nov 22, 2016

Check dict hashing issue #234

Open

added test for JuliaLang/julia#15077

2678dd4

phaverty closed this Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster creation of OrderedDict, revisited #221

Faster creation of OrderedDict, revisited #221

phaverty commented Oct 29, 2016

codecov-io commented Oct 29, 2016 •

edited

Loading

kmsquire Nov 19, 2016

kmsquire Nov 19, 2016

kmsquire Nov 19, 2016

kmsquire Nov 19, 2016

kmsquire Nov 19, 2016

kmsquire Nov 19, 2016

phaverty commented Nov 20, 2016

kmsquire Nov 20, 2016

phaverty commented Nov 21, 2016

kmsquire Nov 21, 2016

phaverty commented Nov 21, 2016

@kmsquire commented on this pull request.

kmsquire commented Nov 22, 2016

phaverty commented Nov 24, 2016

Faster creation of OrderedDict, revisited #221

Faster creation of OrderedDict, revisited #221

Conversation

phaverty commented Oct 29, 2016

codecov-io commented Oct 29, 2016 • edited Loading

Current coverage is 89.67% (diff: 53.65%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phaverty commented Nov 20, 2016

Choose a reason for hiding this comment

phaverty commented Nov 21, 2016

Choose a reason for hiding this comment

phaverty commented Nov 21, 2016

@kmsquire commented on this pull request.

kmsquire commented Nov 22, 2016

phaverty commented Nov 24, 2016

codecov-io commented Oct 29, 2016 •

edited

Loading