Skip to content

Borrowck: refactor visited map to a bitset - #81132

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

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

Conversation

@bugadani

@bugadani bugadani commented Jan 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

rust-highfive commented Jan 17, 2021

Copy link
Copy Markdown
Contributor

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

jonas-schievink commented Jan 18, 2021

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

@bors

bors commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

⌛ Trying commit d04d2bdfa059446cf0da95c009f74a83fe4fd41e with merge 0b6d71c7205bc7c53a47894438791fc10a21a488...

@bors

bors commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

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

bugadani commented Jan 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

rust-timer commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

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

bjorn3 commented Jan 18, 2021

Copy link
Copy Markdown
Member

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

tgnottingham commented Jan 18, 2021

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

@bors

bors commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 4b856c166d1460b32c4f85f27c0fb50ff959ab5e with merge ff419a899d3f37d41aff12dc6410c294b64a4f08...

@bors

bors commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Jan 18, 2021

Copy link
Copy Markdown
Collaborator

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

rust-timer commented Jan 19, 2021

Copy link
Copy Markdown
Collaborator

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
Comment thread compiler/rustc_mir/src/borrow_check/mod.rs Outdated
@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

matthewjasper commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Feb 7, 2021

Copy link
Copy Markdown
Collaborator

📌 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

bugadani commented Feb 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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
Comment thread compiler/rustc_mir/src/dataflow/impls/borrows.rs Outdated

@matthewjasper matthewjasper Feb 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clone is unnecessary here

@bugadani bugadani Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

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

matthewjasper commented Feb 9, 2021

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Feb 9, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 5271c62 has been approved by matthewjasper

@bors

bors commented Feb 9, 2021

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Feb 10, 2021

Copy link
Copy Markdown
Collaborator

☀️ 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