Cleanup and optimize render_impls#157540
Conversation
|
I doubt it'll register on perf, but I do think this should be an optimization (albeit a small one) so let's run perf anyways @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…, r=<try> Cleanup and optimize `render_impls`
This comment has been minimized.
This comment has been minimized.
Docker error |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (19872c0): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing 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 (primary 0.6%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.4%, secondary 3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 514.086s -> 513.359s (-0.14%) |
- take ownership of the `Vec<&Impl>` instead of copying into another alloc - reuse `ImplString` to do natural sort ordering - lazy formatting
1b36eaf to
183502d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Perf results are a pleasant surprise! 🦆 |
|
Nice, well done! r=me once CI pass. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Vec<&Impl>instead of copying into another allocImplStringto do natural sort orderingSomewhat of a follow-up to #157233 and #157179 (cc @nnethercote - thanks!)
This kinda undoes f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it.
r? @GuillaumeGomez
LLM disclosure:
I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that
render_implswill not render anything IFF the list of traits passed to it is empty)