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

perf: Reuse a Vec in mir simplification #68551

Merged
merged 6 commits into from
Mar 12, 2020
Merged

perf: Reuse a Vec in mir simplification #68551

merged 6 commits into from
Mar 12, 2020

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Jan 26, 2020

Just moves the vec out of the outer loop so it is reused every iteration

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2020
@Centril
Copy link
Contributor

Centril commented Jan 26, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jan 26, 2020

⌛ Trying commit 97db376 with merge 874bb3e...

bors added a commit that referenced this pull request Jan 26, 2020
@bors
Auto merge of #68551 - Marwes:allocations_mir, r=<try>
874bb3e 
perf: Reuse a Vec in mir simplification

Just moves the vec out of the outer loop so it is reused every iteration
eddyb
eddyb reviewed Jan 26, 2020
View reviewed changes
src/librustc_mir/transform/simplify.rs Outdated
@@ -124,7 +124,7 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
}

let data = &mut self.basic_blocks[bb];
data.statements.extend(new_stmts);
data.statements.extend(new_stmts.drain(..));
Copy link
Member

Choose a reason for hiding this comment

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

I feel like maybe merge_successor count take bb and modify self.basic_blocks[bb].statements, draining from the original block directly, without any of these copies into a buffer? But that's neither here nor there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, I thought that would be more difficult since there is a risk that the side effect of appending affects something else. Was pretty trivial though when I actually looked at it.

@bors
Copy link
Contributor

bors commented Jan 26, 2020

☀️ Try build successful - checks-azure
Build commit: 874bb3e (874bb3e37d19fa852833270abc711cb823925dea)

@rust-timer
Copy link
Collaborator

Queued 874bb3e with parent 3d8778d, future comparison URL.

@wesleywiser

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@wesleywiser
Copy link
Member

Er...

@rust-timer build 874bb3e

@rust-timer
Copy link
Collaborator

Queued 874bb3e with parent 3d8778d, future comparison URL.

@Marwes
Copy link
Contributor Author

Marwes commented Jan 29, 2020

Perf looks like noise, maybe a slight regression. Might be because there is no specialization on Drain (but there is on IntoIter). This really should just be a clear, minor win otherwise.

@tspiteri
Copy link
Contributor

@Marwes Why not use Vec::append?

@Marwes
Copy link
Contributor Author

Marwes commented Jan 30, 2020

@Marwes Why not use Vec::append?

👍 Wasn't aware that it existed, thanks!

@Marwes
Copy link
Contributor Author

Marwes commented Feb 6, 2020

Could use another part run with the improvements

@wesleywiser
Copy link
Member

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 6, 2020

⌛ Trying commit 3aea9a50f2d262384720050b4a9c1951723f65ff with merge 838480bdac784cac159fd709268227e4f762ef11...

@bors
Copy link
Contributor

bors commented Feb 6, 2020

☀️ Try build successful - checks-azure
Build commit: 838480bdac784cac159fd709268227e4f762ef11 (838480bdac784cac159fd709268227e4f762ef11)

@rust-timer
Copy link
Collaborator

Queued 838480bdac784cac159fd709268227e4f762ef11 with parent 1f8df25, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 838480bdac784cac159fd709268227e4f762ef11, comparison URL.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 2, 2020
@bors
Copy link
Contributor

bors commented Mar 3, 2020

⌛ Testing commit 851e9d6 with merge d867495cc949f2e30f09e34fce5ef2f929ac4b28...

@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Mar 3, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2020
@ecstatic-morse
Copy link
Contributor

Seems spurious.

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2020
@bors
Copy link
Contributor

bors commented Mar 4, 2020

⌛ Testing commit 851e9d6 with merge 118631d...

bors added a commit that referenced this pull request Mar 4, 2020
@bors
Auto merge of #68551 - Marwes:allocations_mir, r=ecstatic-morse
118631d 
perf: Reuse a Vec in mir simplification

Just moves the vec out of the outer loop so it is reused every iteration
@bors
Copy link
Contributor

bors commented Mar 5, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 5, 2020
@ecstatic-morse
Copy link
Contributor

No reason this should time out, so I'll give it one more try. Is this appropriate @pietroalbini?

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2020
@RalfJung
Copy link
Member

RalfJung commented Mar 8, 2020

Since this is explicitly called out as perf-relevant... @bors rollup=never

@bors
Copy link
Contributor

bors commented Mar 12, 2020

⌛ Testing commit 851e9d6 with merge 8780469d4997fb4ac0b4f0741f28200e83196cbd...

@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Mar 12, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 12, 2020
@bors
Copy link
Contributor

bors commented Mar 12, 2020

⌛ Testing commit 851e9d6 with merge 23de827...

@bors
Copy link
Contributor

bors commented Mar 12, 2020

☀️ Test successful - checks-azure
Approved by: ecstatic-morse
Pushing 23de827 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2020
@bors bors merged commit 23de827 into rust-lang:master Mar 12, 2020
@Marwes Marwes deleted the allocations_mir branch March 12, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

@RalfJung RalfJung RalfJung left review comments

@ecstatic-morse ecstatic-morse ecstatic-morse left review comments

Assignees

@matthewjasper matthewjasper

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.