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

Greatly improve performance of sorting dictionaries #5168

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Jan 13, 2022

I noticed in #5163 that dictionary sorting is rather slow and took a stab at improving it.

Dictionary lookup by index is slow, so read all of the keys/values from the dictionary into a vector and sort that instead. Using a dictionary iterator to read the values replaces O(N log N) ClusterTree lookups with O(log N), so this is asymptotically faster. The benchmark is sped up from 3.77 seconds to 21.62 milliseconds, but more reasonably sized Dictionaries will see proportionally smaller benefits.

The downside of this is that the array of Mixed requires 24 bytes of scratch space per element in the dictionary. We already require 8 bytes per element to store the results, so this is just a constant factor increase rather than an asymptotic change in memory use and it's probably not a problem.

@tgoyne tgoyne self-assigned this Jan 13, 2022
@cla-bot cla-bot bot added the cla: yes label Jan 13, 2022
Base automatically changed from tg/sort to master January 14, 2022 16:49
@bmunkholm bmunkholm requested a review from jedelbo September 19, 2022 15:45
@bmunkholm
Copy link
Contributor

@tgoyne @jedelbo Any downsides to getting this reviewed and merged?

@tgoyne
Copy link
Member Author

tgoyne commented Sep 19, 2022

#5780 might make this unnecessary as it significantly changes the performance characteristics of dictionaries. If this is still a meaningful gain then there's probably no downside; the increased scratch memory usage really isn't signficant.

@tgoyne
Copy link
Member Author

tgoyne commented Sep 19, 2022

Still seems worth doing:

next-major:
Req runs:    5  SortIntList (MemOnly, EncryptionOff):         min 179.36ms     max 182.32ms     median 180.32ms     avg 180.77ms     stddev   1.46ms
Req runs:    5  SortIntDictionary (MemOnly, EncryptionOff):   min 687.99ms     max 702.65ms     median 692.61ms     avg 693.63ms     stddev   5.79ms

tg/dict-sort:
Req runs:    5  SortIntList (MemOnly, EncryptionOff):         min 190.65ms     max 202.14ms     median 201.05ms     avg 197.78ms     stddev   5.46ms
Req runs:   10  SortIntDictionary (MemOnly, EncryptionOff):   min  46.27ms     max  48.36ms     median  47.08ms     avg  47.26ms     stddev    762us

next-major + tg/dict-sort:
Req runs:    5  SortIntList (MemOnly, EncryptionOff):         min 182.83ms     max 194.63ms     median 191.08ms     avg 189.38ms     stddev   4.59ms
Req runs:   14  SortIntDictionary (MemOnly, EncryptionOff):   min  33.83ms     max  37.43ms     median  34.86ms     avg  35.24ms     stddev   1.13ms

Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

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

Great! Let's do it.

Dictionary lookup by index is slow, so read all of the keys/values from the
dictionary into a vector and sort that instead. Using a dictionary iterator to
read the values replaces O(N log N) ClusterTree lookups with O(log N), so this
is asymptotically faster. The benchmark is sped up from 3.77 seconds to 21.62
milliseconds, but more reasonably sized Dictionaries will see proportionally
smaller benefits.

The downside of this is that the array of Mixed requires 24 bytes of scratch
space per element in the dictionary. We already require 8 bytes per element to
store the results, so this is just a constant factor increase rather than an
aysmtotic change in memory use and it's probably not a problem.
@tgoyne tgoyne merged commit f81b3ce into master Sep 27, 2022
@tgoyne tgoyne deleted the tg/dict-sort branch September 27, 2022 17:53
tgoyne added a commit that referenced this pull request Sep 27, 2022
…nification

* origin/master:
  Fix a data race in notifier packaging (#5892)
  Install util/http.hpp (#5893)
  Greatly improve performance of sorting dictionaries (#5168)
  Sync client shall not block user writes (#5844)
  update err message check (#5884)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants