Implement ordered dictionary #111
Conversation
|
Alternative to #103 |
funny-falcon
changed the title
|
I didn't fix comments yet, that is why it is marked as [WIP]. |
funny-falcon
force-pushed
the
hash-try2
branch
from
df2ec06 to
35a578a
Compare
funny-falcon
changed the title
|
Looks like it is ready. |
funny-falcon
force-pushed
the
hash-try2
branch
from
35a578a to
6009efe
Compare
|
The master branch is now passing. @funny-falcon: Can you please update yours so we can have a green mark on this branch too? |
benchmark old ns/op new ns/op delta BenchmarkEscapeRune/low_values-4 26.4 26.2 -0.76% BenchmarkEscapeRune/mid_values-4 33.5 33.5 +0.00% BenchmarkEscapeRune/high_values-4 47.6 46.1 -3.15% BenchmarkGetAttr-4 129 127 -1.55% BenchmarkDictSetItem/3-elements-4 926 657 -29.05% BenchmarkDictSetItem/5-elements-4 1302 1256 -3.53% BenchmarkDictSetItem/8-elements-4 2357 1630 -30.84% BenchmarkDictSetItem/12-elements-4 3149 2880 -8.54% BenchmarkDictSetItem/16-elements-4 3930 3312 -15.73% BenchmarkDictSetItem/24-elements-4 7040 5660 -19.60% BenchmarkDictSetItem/32-elements-4 8418 6420 -23.73% BenchmarkDictGetItem-4 181 177 -2.21% BenchmarkDictGetItemBig-4 3164 3139 -0.79% BenchmarkDictIterItems/0-elements-4 921 618 -32.90% BenchmarkDictIterItems/1-elements-4 1020 739 -27.55% BenchmarkDictIterItems/2-elements-4 1151 859 -25.37% BenchmarkDictIterItems/3-elements-4 1296 1020 -21.30% BenchmarkDictIterItems/4-elements-4 1427 1138 -20.25% BenchmarkDictIterItems/5-elements-4 1592 1265 -20.54% BenchmarkDictIterItems/6-elements-4 1874 1386 -26.04% BenchmarkDictIterItems/7-elements-4 1986 1486 -25.18% BenchmarkDictIterItems/8-elements-4 2118 1605 -24.22% BenchmarkDictIterKeys/0-elements-4 914 611 -33.15% BenchmarkDictIterKeys/1-elements-4 1010 701 -30.59% BenchmarkDictIterKeys/2-elements-4 1079 794 -26.41% BenchmarkDictIterKeys/3-elements-4 1205 892 -25.98% BenchmarkDictIterKeys/4-elements-4 1305 984 -24.60% BenchmarkDictIterKeys/5-elements-4 1375 1080 -21.45% BenchmarkDictIterKeys/6-elements-4 1700 1173 -31.00% BenchmarkDictIterKeys/7-elements-4 1787 1284 -28.15% BenchmarkDictIterKeys/8-elements-4 1867 1372 -26.51% BenchmarkDictIterKeys/9-elements-4 1955 1494 -23.58% BenchmarkDictIterValues/0-elements-4 923 617 -33.15% BenchmarkDictIterValues/1-elements-4 929 646 -30.46% BenchmarkDictIterValues/2-elements-4 950 659 -30.63% BenchmarkDictIterValues/3-elements-4 981 693 -29.36% BenchmarkDictIterValues/4-elements-4 1003 716 -28.61% BenchmarkDictIterValues/5-elements-4 1021 744 -27.13% BenchmarkDictIterValues/6-elements-4 1271 776 -38.95% BenchmarkDictIterValues/7-elements-4 1288 804 -37.58% BenchmarkDictIterValues/8-elements-4 1307 825 -36.88% BenchmarkIntNew/interned-4 0.96 0.96 +0.00% BenchmarkIntNew/not_interned-4 70.2 68.5 -2.42% BenchmarkListContains/false-3-4 1261 1032 -18.16% BenchmarkListContains/false-10-4 2263 2031 -10.25% BenchmarkListContains/true-3.1-4 293 291 -0.68% BenchmarkListContains/true-3.3-4 556 552 -0.72% BenchmarkListContains/true-10.10-4 1457 1449 -0.55% BenchmarkNewStr-4 114 115 +0.88% BenchmarkTupleContains/false-3-4 2037 1789 -12.17% BenchmarkTupleContains/false-10-4 4346 4127 -5.04% BenchmarkTupleContains/true-3.1-4 611 608 -0.49% BenchmarkTupleContains/true-3.3-4 1298 1301 +0.23% BenchmarkTupleContains/true-10.10-4 3643 3660 +0.47% benchmark old allocs new allocs delta BenchmarkEscapeRune/low_values-4 1 1 +0.00% BenchmarkEscapeRune/mid_values-4 1 1 +0.00% BenchmarkEscapeRune/high_values-4 1 1 +0.00% BenchmarkGetAttr-4 0 0 +0.00% BenchmarkDictSetItem/3-elements-4 6 3 -50.00% BenchmarkDictSetItem/5-elements-4 8 5 -37.50% BenchmarkDictSetItem/8-elements-4 13 5 -61.54% BenchmarkDictSetItem/12-elements-4 17 8 -52.94% BenchmarkDictSetItem/16-elements-4 21 8 -61.90% BenchmarkDictSetItem/24-elements-4 31 11 -64.52% BenchmarkDictSetItem/32-elements-4 39 11 -71.79% BenchmarkDictGetItem-4 1 1 +0.00% BenchmarkDictGetItemBig-4 1 1 +0.00% BenchmarkDictIterItems/0-elements-4 6 4 -33.33% BenchmarkDictIterItems/1-elements-4 7 5 -28.57% BenchmarkDictIterItems/2-elements-4 8 6 -25.00% BenchmarkDictIterItems/3-elements-4 9 7 -22.22% BenchmarkDictIterItems/4-elements-4 10 8 -20.00% BenchmarkDictIterItems/5-elements-4 11 9 -18.18% BenchmarkDictIterItems/6-elements-4 12 10 -16.67% BenchmarkDictIterItems/7-elements-4 13 11 -15.38% BenchmarkDictIterItems/8-elements-4 14 12 -14.29% BenchmarkDictIterKeys/0-elements-4 6 4 -33.33% BenchmarkDictIterKeys/1-elements-4 6 4 -33.33% BenchmarkDictIterKeys/2-elements-4 6 4 -33.33% BenchmarkDictIterKeys/3-elements-4 6 4 -33.33% BenchmarkDictIterKeys/4-elements-4 6 4 -33.33% BenchmarkDictIterKeys/5-elements-4 6 4 -33.33% BenchmarkDictIterKeys/6-elements-4 6 4 -33.33% BenchmarkDictIterKeys/7-elements-4 6 4 -33.33% BenchmarkDictIterKeys/8-elements-4 6 4 -33.33% BenchmarkDictIterKeys/9-elements-4 6 4 -33.33% BenchmarkDictIterValues/0-elements-4 6 4 -33.33% BenchmarkDictIterValues/1-elements-4 6 4 -33.33% BenchmarkDictIterValues/2-elements-4 6 4 -33.33% BenchmarkDictIterValues/3-elements-4 6 4 -33.33% BenchmarkDictIterValues/4-elements-4 6 4 -33.33% BenchmarkDictIterValues/5-elements-4 6 4 -33.33% BenchmarkDictIterValues/6-elements-4 6 4 -33.33% BenchmarkDictIterValues/7-elements-4 6 4 -33.33% BenchmarkDictIterValues/8-elements-4 6 4 -33.33% BenchmarkIntNew/interned-4 0 0 +0.00% BenchmarkIntNew/not_interned-4 1 1 +0.00% BenchmarkListContains/false-3-4 7 5 -28.57% BenchmarkListContains/false-10-4 7 5 -28.57% BenchmarkListContains/true-3.1-4 2 2 +0.00% BenchmarkListContains/true-3.3-4 2 2 +0.00% BenchmarkListContains/true-10.10-4 2 2 +0.00% BenchmarkNewStr-4 1 1 +0.00% BenchmarkTupleContains/false-3-4 8 6 -25.00% BenchmarkTupleContains/false-10-4 8 6 -25.00% BenchmarkTupleContains/true-3.1-4 3 3 +0.00% BenchmarkTupleContains/true-3.3-4 3 3 +0.00% BenchmarkTupleContains/true-10.10-4 3 3 +0.00% benchmark old bytes new bytes delta BenchmarkEscapeRune/low_values-4 4 4 +0.00% BenchmarkEscapeRune/mid_values-4 8 8 +0.00% BenchmarkEscapeRune/high_values-4 16 16 +0.00% BenchmarkGetAttr-4 0 0 +0.00% BenchmarkDictSetItem/3-elements-4 272 192 -29.41% BenchmarkDictSetItem/5-elements-4 336 416 +23.81% BenchmarkDictSetItem/8-elements-4 736 416 -43.48% BenchmarkDictSetItem/12-elements-4 864 960 +11.11% BenchmarkDictSetItem/16-elements-4 992 960 -3.23% BenchmarkDictSetItem/24-elements-4 2320 2016 -13.10% BenchmarkDictSetItem/32-elements-4 2576 2016 -21.74% BenchmarkDictGetItem-4 32 32 +0.00% BenchmarkDictGetItemBig-4 32 32 +0.00% BenchmarkDictIterItems/0-elements-4 320 208 -35.00% BenchmarkDictIterItems/1-elements-4 384 272 -29.17% BenchmarkDictIterItems/2-elements-4 448 336 -25.00% BenchmarkDictIterItems/3-elements-4 512 400 -21.88% BenchmarkDictIterItems/4-elements-4 576 464 -19.44% BenchmarkDictIterItems/5-elements-4 640 528 -17.50% BenchmarkDictIterItems/6-elements-4 704 592 -15.91% BenchmarkDictIterItems/7-elements-4 768 656 -14.58% BenchmarkDictIterItems/8-elements-4 832 720 -13.46% BenchmarkDictIterKeys/0-elements-4 320 208 -35.00% BenchmarkDictIterKeys/1-elements-4 320 208 -35.00% BenchmarkDictIterKeys/2-elements-4 320 208 -35.00% BenchmarkDictIterKeys/3-elements-4 320 208 -35.00% BenchmarkDictIterKeys/4-elements-4 320 208 -35.00% BenchmarkDictIterKeys/5-elements-4 320 208 -35.00% BenchmarkDictIterKeys/6-elements-4 320 208 -35.00% BenchmarkDictIterKeys/7-elements-4 320 208 -35.00% BenchmarkDictIterKeys/8-elements-4 320 208 -35.00% BenchmarkDictIterKeys/9-elements-4 320 208 -35.00% BenchmarkDictIterValues/0-elements-4 320 208 -35.00% BenchmarkDictIterValues/1-elements-4 320 208 -35.00% BenchmarkDictIterValues/2-elements-4 320 208 -35.00% BenchmarkDictIterValues/3-elements-4 320 208 -35.00% BenchmarkDictIterValues/4-elements-4 320 208 -35.00% BenchmarkDictIterValues/5-elements-4 320 208 -35.00% BenchmarkDictIterValues/6-elements-4 320 208 -35.00% BenchmarkDictIterValues/7-elements-4 320 208 -35.00% BenchmarkDictIterValues/8-elements-4 320 208 -35.00% BenchmarkIntNew/interned-4 0 0 +0.00% BenchmarkIntNew/not_interned-4 32 32 +0.00% BenchmarkListContains/false-3-4 336 224 -33.33% BenchmarkListContains/false-10-4 336 224 -33.33% BenchmarkListContains/true-3.1-4 80 80 +0.00% BenchmarkListContains/true-3.3-4 80 80 +0.00% BenchmarkListContains/true-10.10-4 80 80 +0.00% BenchmarkNewStr-4 48 48 +0.00% BenchmarkTupleContains/false-3-4 400 288 -28.00% BenchmarkTupleContains/false-10-4 400 288 -28.00% BenchmarkTupleContains/true-3.1-4 144 144 +0.00% BenchmarkTupleContains/true-3.3-4 144 144 +0.00% BenchmarkTupleContains/true-10.10-4 144 144 +0.00%
funny-falcon
force-pushed
the
hash-try2
branch
from
6009efe to
0d3df85
Compare
There was a problem hiding this comment.
If I understood correctly, a dict that is looped being inserted than popped will grow in waste of memory and time of iteration, being O(n) with n = number of unique keys every used on this dict.
- Is there a way to mitigate this?
- This is how CPython does? How do they fix this problem?
|
Not indefinitely, it is bound by O(n), where n is number of (unique) keys. And constant is usually 2, but could be a bit larger (4). Ruby's hash has also tracker for first entry, because Ruby's hash has |
|
tests passed. |
|
Note that iteration over unordered dictionary still suffer from skipping empty and removed elements. Therefore there is no much difference. |
|
There could be euristic on delete: |
| index, entry, raised := t.lookupEntry(f, hashv, key) | ||
| if raised != nil { | ||
| d.mutex.Unlock(f) | ||
| return nil, raised |
There was a problem hiding this comment.
Can all this d.mutex.Unlock(f) before a return be replaced by a defer right after the d.mutex.Lock() on line 491 instead?
There was a problem hiding this comment.
defer is not free. Some time ago it even costed. But probably, putItem is too expensive itself.
I will run benchmark, and if you considered, difference doesn't matter, then I will.
There was a problem hiding this comment.
As I've said, defer is not free:
BenchmarkDictSetItem/3-elements-4 658 874 +32.83%
BenchmarkDictSetItem/5-elements-4 1271 1599 +25.81%
BenchmarkDictSetItem/8-elements-4 1609 2153 +33.81%
BenchmarkDictSetItem/12-elements-4 2845 3616 +27.10%
BenchmarkDictSetItem/16-elements-4 3301 4347 +31.69%
BenchmarkDictSetItem/24-elements-4 5673 6999 +23.37%
BenchmarkDictSetItem/32-elements-4 6516 8452 +29.71%
Do you really think it has to be in such hot place?
There was a problem hiding this comment.
Note, that i've tested on i5-5100 , and it is quite old processor today.
Could some one test on more modern one to prove or disprove this proportion?
There was a problem hiding this comment.
This is odd. I would expect both to be equivalent and act like a macro for the compiler, putting it before the returns for us.
Now I vote for let it be as is. Someday will be better. This day we replace everything with a defer.
Can you please add a code comment about this decision, for the next coder?
There was a problem hiding this comment.
Yes, I did in the next commit.
| d.incVersion() | ||
| if d.table.used == 0 { | ||
| d.mutex.Unlock(f) | ||
| return nil, f.RaiseType(KeyErrorType, "popitem(): dictionary is empty") |
There was a problem hiding this comment.
Again, what about defer d.mutex.Unlock(f) right after the Lock() call?
There was a problem hiding this comment.
As I think dict.popitem is quite useless method, ok I will.
| if hasattr(sys, "goversion"): | ||
| assert l == ['foo', 'bar', 'baz', 'qux'] | ||
| else: | ||
| assert l == ['baz', 'foo', 'bar', 'qux'] |
There was a problem hiding this comment.
This is testing that Grumpy have ordered dict or not ordered dict?
There was a problem hiding this comment.
It checks that grumpy has order-preserving dictionary, and python2.7 has unordered dictionary.
Order for grumpy I wrote looking at assignments above.
I didn't add it at Dict.putItem yet, because it is hot place. Probably in a future it will be considered to be safer.
|
Is iteration is important? I have patch that improves start of iteration at the expense of 2-4% slower putItem. I'd prefer for faster putItem, that is why I didn't push it. |
|
If there is no complains, lets merge it on monday, ok? |
Implements order-preserving dictionary a-la python 3.7.
[]*dictEntryto[]dictEntry, store entries in order and use index hash table for hash lookup.Memory ordering now relies:
-- table is stored last after resize
--
indexis written after new entry is written, andfillincremented thenNote that nil checks are free, because compiler inserted hidden nil checks could be omitted then.
While DictGetItem and DictGetItemBig shows improvement, I think there is no real difference, because it is
benchcmp -bestof 3 runs, and results varies much from run to run.Iteration is much faster because dictTable is not allocated for empty dict, and StopIteration exception is created with dict. Though without this optimization it is still a bit faster than master, but not that much.