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

Optimize sort followed by limit #6941

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Optimize sort followed by limit #6941

merged 2 commits into from
Sep 7, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Sep 1, 2023

If the limit is much smaller than the total size of the TableView, then it is faster to make a sorted insert into a vector that is kept at the limit size.

What, How & Why?

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

If the limit is much smaller than the total size of the TableView, then
it is faster to make a sorted insert into a vector that is kept at the
limit size.
@ironage
Copy link
Contributor

ironage commented Sep 1, 2023

Once we do #6933 we should be able to optimize this further by only inserting the limit number of elements into the vector in the first place instead of having an extra buffer like you do here. Given that we expect future optimizations, could you please move the benchmark out of the test and into benchmark-common-tasks so that we can track this case over time. If you could post that benchmark's results compared to current master, I'd also be curious to see what kind of improvement this actually is 😄
The actual code changes seem fine to me though; this is a sorely needed optimization 👍

IndexPairs buffer;
buffer.reserve(limit + 1);
for (auto& elem : v) {
auto it = std::lower_bound(buffer.begin(), buffer.end(), elem, predicate);
Copy link
Member

Choose a reason for hiding this comment

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

If std::ref() is doing something useful in the std::sort() case then it will here too (it makes it so that the predicate is not copied when it's passed to the algorithm).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jedelbo
Copy link
Contributor Author

jedelbo commented Sep 4, 2023

@ironage I am not sure how we can avoid the buffer. We cannot clear the buffer that holds the elements we should sort.
I have added a benchmark. You should be aware the we can get any performance gain we want by carefully selecting the original table view size and limit .In the test I added, the size and limit is 10000 and 100 respectively. This is the result

Req runs:  387  SortThenLimit (MemOnly, EncryptionOff):       min   1.26ms (-36.01%)           max   1.33ms (-35.29%)           med   1.28ms (-35.62%)           avg   1.28ms (-35.59%)           stddev    14us (-17.69%)      

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

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

👍

@jedelbo jedelbo merged commit 14215e8 into master Sep 7, 2023
24 of 27 checks passed
@jedelbo jedelbo deleted the je/sort-limit branch September 7, 2023 09:37
@jedelbo jedelbo mentioned this pull request Oct 5, 2023
@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