-
-
Notifications
You must be signed in to change notification settings - Fork 767
Reduce sorting in TopDocs #2646
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
base: main
Are you sure you want to change the base?
Conversation
let (_, _, remainder) = self.buffer.select_nth_unstable(offset); | ||
remainder.sort_unstable(); | ||
self.buffer.into_iter().skip(offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced this is actually beneficial, do you have any benchmarks demonstrating that doing these two effective sorts is more efficient than the one sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select_nth
is not a sort: it only pivots elements around the nth
position (in linear time).
But no: I have not run benchmarks! Does CI run a benchmark suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would create a set of micro benchmarks between the two changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For small values of offset, it is probably harmful.
For high values of offset, it is probably helpful.
...
Can we have unit tests?
There are benchmarks but we do not run them in CI.
4526fc0
to
267b41b
Compare
I've added some microbenchmarks, but they're pretty clearly in the wrong place: should I break out a separate benchmark target? Note though that the fact that the microbenchmarks were only testing a single segment was probably not realistic for the Splitting the two changes into two commits, I see: For
For
So unless the |
@@ -73,6 +75,12 @@ fn bench_agg(mut group: InputGroup<Index>) { | |||
register!(group, histogram_with_avg_sub_agg); | |||
register!(group, avg_and_range_with_avg_sub_agg); | |||
|
|||
register!(group, top_docs_small_shallow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put that in a different file? collector_bench for instance?
vec![ | ||
(0.8, DocAddress::new(0, 1)), | ||
(0.2, DocAddress::new(0, 3)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not fond of the assert now. The order is now very specific to the implementation. Can you assert on the set. For instance, by sorting the results (in the unit test.)
(0.3, DocAddress::new(0, 5)), | ||
(0.2, DocAddress::new(0, 3)) | ||
(0.9, DocAddress::new(0, 7)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Reduce sorting in
TopDocs
by:offset
-- we can pivot instead.