Skip to content
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

Simplified insert_phase_2 #3

Merged
merged 2 commits into from Oct 22, 2016

Conversation

Projects
None yet
2 participants
@pczarn
Copy link
Contributor

commented Oct 22, 2016

Since the removal operation is an inverse of insertion, removal does back-shift, and back-shift is an inverse of forward-shift ... you get the meaning of this change. The phase 2 of insertion can be just a forward-shift.

The new behavior displaces all elements by 1, in contrast to the old behavior which may displace some elements by more than 1 and leave the rest in the same place.

I didn't include the commit pczarn@a64f0fd, which doesn't improve anything. If you have time, please look into generated code. With that third commit, there might be a way to merge 2 of 3 branches of the control flow with some elaborate trick.

Now benchmarks. I'm not sure why lookups seem to get faster. I think their timings have high variance.

 name                                   with-0 ns/iter  with-2 ns/iter  diff ns/iter  diff %
 entry_orderedmap_150                   4,494           4,338                   -156  -3.47% 
 grow_fnv_ordermap_100_000              7,128,069       6,740,141           -387,928  -5.44% 
 insert_orderedmap_100_000              5,453,012       5,525,914             72,902   1.34% 
 insert_orderedmap_10_000               378,007         366,926              -11,081  -2.93% 
 insert_orderedmap_150                  4,866           4,664                   -202  -4.15% 
 insert_orderedmap_int_bigvalue_10_000  659,516         632,560              -26,956  -4.09% 
 insert_orderedmap_str_10_000           452,410         435,960              -16,450  -3.64% 
 insert_orderedmap_string_10_000        1,472,525       1,415,280            -57,245  -3.89% 
 iterate_orderedmap_10_000              8,727           8,727                      0   0.00% 
 lookup_hashmap_100_000_inorder_multi   236,324         237,400                1,076   0.46% 
 lookup_orderedmap_10_000_exist         194,823         194,504                 -319  -0.16% 
 lookup_orderedmap_10_000_noexist       189,026         188,407                 -619  -0.33% 
 lookup_ordermap_100_000_inorder_multi  180,149         173,545               -6,604  -3.67% 
 lookup_ordermap_100_000_multi          253,593         228,990              -24,603  -9.70% 
 lookup_ordermap_100_000_single         70              64                        -6  -8.57% 
 new_orderedmap                         5               5                          0   0.00% 
 ordermap_merge_shuffle                 968,587         925,674              -42,913  -4.43% 
 ordermap_merge_simple                  684,382         641,158              -43,224  -6.32% 
 with_capacity_10e5_orderedmap          34,727          34,488                  -239  -0.69%

This change is Reviewable

@bluss bluss self-assigned this Oct 22, 2016

@bluss

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2016

Thanks a lot! Do you know if there are any benefits with the old swapping algorithm? I can't really see it doing much difference.

@bluss bluss merged commit 8f7ebde into bluss:master Oct 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bluss

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2016

in version 0.2.4

@pczarn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2016

The only benefit of the old algorithm is a potentially smaller number of memory writes. I suppose that only matters when entries are big, but here they always have 8 bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.