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.

Sign up for GitHub

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

Jump to bottom

rustc: return iterators from Terminator(Kind)::successors(_mut). #50278

Merged
merged 1 commit into from
May 2, 2018
Merged

rustc: return iterators from Terminator(Kind)::successors(_mut). #50278

merged 1 commit into from
May 2, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 27, 2018

Minor cleanup (and potentially speedup) prompted by @nnethercote's SmallVec experiments.
This PR assumes .count() and .nth(i) on iter::Chain<option::IntoIter, slice::Iter(Mut)> are O(1), but otherwise all of the uses appear to immediately iterate through the successors.

r? @nikomatsakis

@rust-lang rust-lang deleted a comment from rust-highfive Apr 27, 2018
@eddyb eddyb force-pushed the mir-succ-iter branch 2 times, most recently from bcff042 to fed7cef Compare April 27, 2018 14:32
@rust-lang rust-lang deleted a comment from rust-highfive Apr 27, 2018
@arielb1
Copy link
Contributor

arielb1 commented Apr 27, 2018

Is this supposed to bring any measurable improvements?

arielb1
arielb1 reviewed Apr 27, 2018
View reviewed changes
src/librustc/mir/mod.rs Outdated
@@ -1952,7 +1951,8 @@ impl<'tcx> ControlFlowGraph for Mir<'tcx> {
fn successors<'graph>(&'graph self, node: Self::Node)
-> <Self as GraphSuccessors<'graph>>::Iter
{
self.basic_blocks[node].terminator().successors().into_owned().into_iter()
self.basic_blocks[node].terminator().successors()
.cloned().collect::<Vec<_>>().into_iter()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the .collect().into_iter()?

I think the answer is "that would require us to not use impl Iterator, but rather the concrete type for successors - or otherwise implement impl Trait in impls". The concrete type is not that ugly I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have the concrete type in an exported type to emulate typeof.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, we could have Successors and SuccessorsMut. I'll make the change next week.

arielb1
arielb1 reviewed Apr 27, 2018
View reviewed changes
src/librustc/mir/traversal.rs Outdated
@@ -127,7 +127,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {
if let Some(ref term) = data.terminator {
po.visited.insert(root.index());

let succs = term.successors().into_owned().into_iter();
let succs = term.successors().cloned().collect::<Vec<_>>().into_iter();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

arielb1
arielb1 reviewed Apr 27, 2018
View reviewed changes
src/librustc/mir/traversal.rs Outdated
@@ -197,7 +197,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {

if self.visited.insert(bb.index()) {
if let Some(ref term) = self.mir[bb].terminator {
let succs = term.successors().into_owned().into_iter();
let succs = term.successors().cloned().collect::<Vec<_>>().into_iter();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@arielb1
Copy link
Contributor

arielb1 commented Apr 27, 2018 •
edited
Loading

This PR assumes .count() and .nth(i) on iter::Chain<option::IntoIter, slice::Iter(Mut)> are O(1)

Not even that for count. In that Display function we only call count once to traverse.

@nnethercote
Copy link
Contributor

I was looking at this because the allocations done by successors_mut were showing up fairly high in DHAT profiling output for some of the rustc-perf benchmarks, esp. inflate, syn, and style-servo.

I tried changing the return type to a SmallVec with 8 elements. This successfully avoided the allocation in >99.9% of the cases, but resulted in negligible speedups. For example, for inflate the total number of allocations dropped by 5% but the instruction count only dropped by 0.1%.

Overall, it probably won't make a difference to speed, but it does reduce allocations and so can't hurt. It also makes the code of successors and successors_mut nicer and more consistent.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2018
@nikomatsakis
Copy link
Contributor

r=me with arielb1's suggestion

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018
@eddyb eddyb force-pushed the mir-succ-iter branch from fed7cef to f0f26b8 Compare May 1, 2018 10:12
@eddyb
Copy link
Member Author

eddyb commented May 1, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 1, 2018

📌 Commit f0f26b8 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2018
@eddyb eddyb changed the title rustc: return impl Iterator from Terminator(Kind)::successors(_mut). rustc: return iterators from Terminator(Kind)::successors(_mut). May 1, 2018
@bors
Copy link
Contributor

bors commented May 2, 2018

⌛ Testing commit f0f26b8 with merge a272684...

bors added a commit that referenced this pull request May 2, 2018
@bors
Auto merge of #50278 - eddyb:mir-succ-iter, r=nikomatsakis
a272684 
rustc: return iterators from Terminator(Kind)::successors(_mut).

Minor cleanup (and potentially speedup) prompted by @nnethercote's `SmallVec` experiments.
This PR assumes `.count()` and `.nth(i)` on `iter::Chain<option::IntoIter, slice::Iter(Mut)>` are `O(1)`, but otherwise all of the uses appear to immediately iterate through the successors.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a272684 to master...

@bors bors merged commit f0f26b8 into rust-lang:master May 2, 2018
@eddyb eddyb deleted the mir-succ-iter branch May 2, 2018 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arielb1 arielb1 arielb1 left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eddyb @arielb1 @nnethercote @nikomatsakis @bors @Mark-Simulacrum @pietroalbini