Skip to content

Borrowck: refactor visited map to a bitset#81132

Merged
bors merged 2 commits intorust-lang:masterfrom
bugadani:map-prealloc
Feb 10, 2021
Merged

Borrowck: refactor visited map to a bitset#81132
bors merged 2 commits intorust-lang:masterfrom
bugadani:map-prealloc

Conversation

@bugadani
Copy link
Contributor

@bugadani bugadani commented Jan 17, 2021 •
edited
Loading

This PR refactors Borrows and the precompute_borrows_out_of_scope function so that this initial phase has a much reduced memory pressure. This is achieved by reducing what is stored on the heap, and also reusing heap memory as much as possible.

@rust-highfive
Copy link
Contributor

rust-highfive commented Jan 17, 2021

r? @matthewjasper

(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 17, 2021
@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jan 18, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Jan 18, 2021

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 18, 2021

⌛ Trying commit d04d2bdfa059446cf0da95c009f74a83fe4fd41e with merge 0b6d71c7205bc7c53a47894438791fc10a21a488...

@bors
Copy link
Collaborator

bors commented Jan 18, 2021

☀️ Try build successful - checks-actions
Build commit: 0b6d71c7205bc7c53a47894438791fc10a21a488 (0b6d71c7205bc7c53a47894438791fc10a21a488)

@rust-timer
Copy link
Collaborator

rust-timer commented Jan 18, 2021

Queued 0b6d71c7205bc7c53a47894438791fc10a21a488 with parent 4253153, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2021
@bugadani
Copy link
Contributor Author

bugadani commented Jan 18, 2021 •
edited
Loading

That map can actually be a set (lo is only used for the first bb), so I'll have more changes on this.

@rust-timer
Copy link
Collaborator

rust-timer commented Jan 18, 2021

Finished benchmarking try commit (0b6d71c7205bc7c53a47894438791fc10a21a488): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2021
@bjorn3
Copy link
Member

bjorn3 commented Jan 18, 2021

The difference is indistinguishable from noise.

@bugadani bugadani changed the title Borrowck: reserve space for visited map Borrowck: refactor visited map to a bitset Jan 18, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgnottingham
Copy link
Contributor

tgnottingham commented Jan 18, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Jan 18, 2021

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 18, 2021

⌛ Trying commit 4b856c166d1460b32c4f85f27c0fb50ff959ab5e with merge ff419a899d3f37d41aff12dc6410c294b64a4f08...

@bors
Copy link
Collaborator

bors commented Jan 18, 2021

☀️ Try build successful - checks-actions
Build commit: ff419a899d3f37d41aff12dc6410c294b64a4f08 (ff419a899d3f37d41aff12dc6410c294b64a4f08)

@rust-timer
Copy link
Collaborator

rust-timer commented Jan 18, 2021

Queued ff419a899d3f37d41aff12dc6410c294b64a4f08 with parent 73f233b, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2021
@rust-timer
Copy link
Collaborator

rust-timer commented Jan 19, 2021

Finished benchmarking try commit (ff419a899d3f37d41aff12dc6410c294b64a4f08): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2021
matthewjasper
matthewjasper reviewed Feb 7, 2021
View reviewed changes
@bugadani
Optimize Borrows
46f3045 
Reuse as much memory as possible, reduce number of allocations.
Use BitSet instead of a HashMap, since only a single bit of
information was used as the map's value.
@matthewjasper
Copy link
Contributor

matthewjasper commented Feb 7, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 7, 2021

📌 Commit 46f3045 has been approved by matthewjasper

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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 Feb 7, 2021
@bugadani
Copy link
Contributor Author

bugadani commented Feb 7, 2021 •
edited
Loading

@matthewjasper sorry I snuck in an after-last-minute change, Borrows doesn't need to take RCs at all. Also turns out I should've checked what I committed. Oh well.

matthewjasper
matthewjasper reviewed Feb 8, 2021
View reviewed changes
compiler/rustc_mir/src/dataflow/impls/borrows.rs Outdated
Copy link
Contributor

@matthewjasper matthewjasper Feb 8, 2021

Choose a reason for hiding this comment

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

Clone is unnecessary here

Copy link
Contributor Author

@bugadani bugadani Feb 8, 2021

Choose a reason for hiding this comment

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

OMG thanks for noticing!

@matthewjasper
Copy link
Contributor

matthewjasper commented Feb 9, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

📌 Commit 5271c62 has been approved by matthewjasper

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

⌛ Testing commit 5271c62 with merge 87bacf2...

@bors
Copy link
Collaborator

bors commented Feb 10, 2021

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing 87bacf2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2021
@bors bors merged commit 87bacf2 into rust-lang:master Feb 10, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 10, 2021
This was referenced Feb 10, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewjasper matthewjasper matthewjasper left review comments

Assignees

@matthewjasper matthewjasper

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.52.0

Development

Successfully merging this pull request may close these issues.

10 participants

@bugadani @rust-highfive @jonas-schievink @rust-timer @bors @bjorn3 @rust-log-analyzer @tgnottingham @matthewjasper @rustbot