Remove `scope_auxiliary`. #37764

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
7 participants
@nnethercote
Contributor

nnethercote commented Nov 14, 2016

scope_auxiliary is a big part of the high memory usage in #36799. It's only used for MIR dumping. I have taken a hubristic approach: I have assumed that particular use is unimportant and removed scope_auxiliary and related things. This reduces peak RSS by ~10% for a cut-down version of the program in #36799.

If that assumption is wrong perhaps we can avoid building scope_auxiliary unless MIR dumping is enabled.

Remove `scope_auxiliary`.
This reduces the peak RSS for a cut-down version of the program
in #36799 by 10%, from 951MB to 856MB.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 14, 2016

Collaborator

r? @nikomatsakis

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

Collaborator

rust-highfive commented Nov 14, 2016

r? @nikomatsakis

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

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 14, 2016

Contributor

I think we decided we are going to take a different approach on the MIR size thing.

Contributor

arielb1 commented Nov 14, 2016

I think we decided we are going to take a different approach on the MIR size thing.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 14, 2016

Contributor

@arielb1 did we have another use in mind for scope-auxiliary? I think it is not needed (the MIR dumping is, I think, just so that we can see what was being generated).

I had originally thought we would use it for borrowck -- and I still want to do a version of borrowck that uses scopes as a first step. @pnkfelix is supposed to be investigating. However, we had agreed we'd just try to add in the scopes that we actually need -- i.e., those that are targeted by a borrow.

Seems fine to remove what we have (esp. for a 10% win) and come back with a clean-slate approach.

cc @rust-lang/compiler -- thoughts?

Contributor

nikomatsakis commented Nov 14, 2016

@arielb1 did we have another use in mind for scope-auxiliary? I think it is not needed (the MIR dumping is, I think, just so that we can see what was being generated).

I had originally thought we would use it for borrowck -- and I still want to do a version of borrowck that uses scopes as a first step. @pnkfelix is supposed to be investigating. However, we had agreed we'd just try to add in the scopes that we actually need -- i.e., those that are targeted by a borrow.

Seems fine to remove what we have (esp. for a 10% win) and come back with a clean-slate approach.

cc @rust-lang/compiler -- thoughts?

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 15, 2016

Member

I am fine with removing it if MIR borrowck won't use it.

Member

eddyb commented Nov 15, 2016

I am fine with removing it if MIR borrowck won't use it.

@brson brson added the relnotes label Nov 15, 2016

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
Contributor

nikomatsakis commented Nov 15, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 15, 2016

Contributor

📌 Commit d775570 has been approved by nikomatsakis

Contributor

bors commented Nov 15, 2016

📌 Commit d775570 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 15, 2016

Contributor

let's do it. we can always put it back.

Contributor

nikomatsakis commented Nov 15, 2016

let's do it. we can always put it back.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 16, 2016

Contributor

⌛️ Testing commit d775570 with merge c19cb9c...

Contributor

bors commented Nov 16, 2016

⌛️ Testing commit d775570 with merge c19cb9c...

bors added a commit that referenced this pull request Nov 16, 2016

Auto merge of #37764 - nnethercote:shrink-scope_auxiliary, r=nikomats…
…akis

Remove `scope_auxiliary`.

`scope_auxiliary` is a big part of the high memory usage in #36799. It's only used for MIR dumping. I have taken a hubristic approach: I have assumed that particular use is unimportant and removed `scope_auxiliary` and related things. This reduces peak RSS by ~10% for a cut-down version of the program in #36799.

If that assumption is wrong perhaps we can avoid building `scope_auxiliary` unless MIR dumping is enabled.

@bors bors merged commit d775570 into rust-lang:master Nov 16, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:shrink-scope_auxiliary branch Nov 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment