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
Merged

Conversation

pczarn
Copy link
Contributor

@pczarn pczarn 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
Copy link
Member

bluss 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 indexmap-rs:master Oct 22, 2016
@bluss
Copy link
Member

bluss commented Oct 22, 2016

in version 0.2.4

@pczarn
Copy link
Contributor Author

pczarn 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants