remove all 4 RC clones in min_candidates. allowing it to be inlined #5625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors: r+ Nice find! Generally inlined code is faster in that is avoids setting up a call frame and primarily enables further optimizations, but if the runtime is faster it's faster, so no need to worry about things like |
|
📌 Commit edc516c has been approved by |
bors
added a commit
that referenced
this pull request
Jun 27, 2018
remove all 4 RC clones in min_candidates. allowing it to be inlined So I was looking at a profile, and noted that `DepsFrame::min_candidates` was taking ~10% of the runtime. The odd part is that it should be a thin wrapper around `Vec::len()`, and so should be completely inlined away. Also it is the key for the `BinaryHeap` so it gets called a lot! Looking into it `remaining_siblings.clone()` clones the RC in the `RcVecIter` then `.next()` clones `T` witch is a `DepInfo` each part of which is an RC that needs to be cloned. All 4 of these RC clones can be removed, but it is apparently too much for the optimizer. So I added a 'peek' method that uses a normal reference to the inner value instead of an RC clone. After this `DepsFrame::min_candidates` does not appear in the profile results. Probably as the name is inlined away. But is the inlined code faster? before: 20000000 ticks, 104s, 192.308 ticks/ms after: 20000000 ticks, 87s, 229.885 ticks/ms So yes ~16% faster! All profiling/benchmark was done by commenting out the code from #5213 so its test case would run for a long time. But this should improve the happy path as well.
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So I was looking at a profile, and noted that
DepsFrame::min_candidateswas taking ~10% of the runtime. The odd part is that it should be a thin wrapper aroundVec::len(), and so should be completely inlined away. Also it is the key for theBinaryHeapso it gets called a lot! Looking into itremaining_siblings.clone()clones the RC in theRcVecIterthen.next()clonesTwitch is aDepInfoeach part of which is an RC that needs to be cloned. All 4 of these RC clones can be removed, but it is apparently too much for the optimizer. So I added a 'peek' method that uses a normal reference to the inner value instead of an RC clone. After thisDepsFrame::min_candidatesdoes not appear in the profile results. Probably as the name is inlined away. But is the inlined code faster?before: 20000000 ticks, 104s, 192.308 ticks/ms
after: 20000000 ticks, 87s, 229.885 ticks/ms
So yes ~16% faster!
All profiling/benchmark was done by commenting out the code from #5213 so its test case would run for a long time. But this should improve the happy path as well.