-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Skip find_existing_run
call if head and tail pairs sorted differently
#143495
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: master
Are you sure you want to change the base?
Skip find_existing_run
call if head and tail pairs sorted differently
#143495
Conversation
Failed to set assignee to
|
r? libs |
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…_run_detection_in_sort_unstable, r=<try> Skip `find_existing_run` call if head and tail pairs sorted differently This would help to avoid running comparator for all elements when user pushed an element to end of sorted Vec, breaking order only in last element. r? `@Voultapher`
This comment has been minimized.
This comment has been minimized.
@bors2 try cancel |
@AngelicosPhosphoros: 🔑 Insufficient privileges: not in try users |
This would help to avoid running comparator for all elements when user pushed an element to end of sorted Vec, breaking order only in last element.
5735153
to
67ac435
Compare
Finished benchmarking commit (f359b11): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.2%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 461.008s -> 460.509s (-0.11%) |
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…_run_detection_in_sort_unstable, r=<try> Skip `find_existing_run` call if head and tail pairs sorted differently This would help to avoid running comparator for all elements when user pushed an element to end of sorted Vec, breaking order only in last element. r? `@Voultapher`
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (0ccfeea): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.1%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 462.855s -> 461.018s (-0.40%) |
I'll try to look at this PR coming week. |
@AngelicosPhosphoros thanks for tagging me, here is a review of the idea rather than the code itself, which is fine other than maybe the h0 etc. variable names. I can totally understand the motivation for this change. Why waste N-2 comparisons if it can be avoided by doing 2-4 additional comparisons? At first glance it's a nice algorithmic improvement with no downsides, but upon closer inspection it leaves me with mixed feelings. Before going further let's look at some real performance figures, since the motivation for this change is rooted in improving performance. All tests were performed with As we can see there is a small but noticeable improvement for Zooming out, there is a larger issue. Essentially this is trying to optimize a known and documented performance sub-optimality in a way that only works for a very narrow use-case. The documentation for
If users can predict this use-case they are much better served with With all this combined I'm not convinced that this change - which represents a small but non-zero increase in code complexity - should be merged. It's a non-ideal situation that the generally faster |
This would help to avoid running comparator for all elements when user pushed an element to end of sorted Vec, breaking order only in last element.
r? @Voultapher