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.

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

Debugflag: -Z emit-end-regions #44249

Merged
merged 4 commits into from
Sep 7, 2017

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Sep 1, 2017

Skip EndRegion emission by default. Use -Z emit-end-regions to reenable it.

The main intent is to fix cases where EndRegion emission is believed to be causing excess peak memory pressure.

It may also be a welcome change to people inspecting the MIR output who find the EndRegions to be a distraction.

(In later follow-up PR's I will put in safe-guards against using the current mir-borrowck without enabling EndRegion emission. But I wanted this PR to be minimal, in part because we may wish to backport it to the beta channel if we find that it reduces peak memory usage significantly.)

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 1, 2017

I switched to the approach of turning off EndRegion by default because I was getting some potentially worrisome results while looking into some regressions with respect to rustc peak memory usage (#43834, #43827, #43126), and @nikomatsakis and I agreed that this was the simplest path forward.

Why is this is right path: the EndRegion code is solely for experimentation purposes at this point, anyone doing such experiments (namely myself and @RalfJung) should be in reasonable positions to use a debug flag to re-enable their emission when desired.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2017
@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2017

Could we have -Zmir-emit-validate imply -Zemit-end-regions? Validation needs EndRegions, it doesn't make any sense otherwise.

Also maybe the two flags, being similar in nature, should have similar names. E.g., the validation flag could be renamed to -Zemit-validate-mir or so.

@bors
Copy link
Contributor

bors commented Sep 3, 2017

☔ The latest upstream changes (presumably #44253) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 4, 2017

@RalfJung I figured changes like that could wait for a follow-up PR; I want this one to be somewhat minimal so that it is easier to back port. Do you have any objection to that plan?

@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2017 via email

…able it.

The main intent is to fix cases where EndRegion emission is believed
to be causing excess peak memory pressure.

It may also be a welcome change to people inspecting the MIR output
who find the EndRegions to be a distraction.
@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 5, 2017

Okay I'll see about making -Z mir-emit-validate imply -Z emit-end-regions

…N > 0).

This way the miri test suite does not have to be updated to explcitly
request `-Z emit-end-regions`.
@RalfJung
Copy link
Member

RalfJung commented Sep 5, 2017

Looks good, thanks!

@arielb1
Copy link
Contributor

arielb1 commented Sep 5, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Sep 5, 2017

📌 Commit f2892ad has been approved by arielb1

@arielb1 arielb1 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 Sep 5, 2017
@bors
Copy link
Contributor

bors commented Sep 7, 2017

⌛ Testing commit f2892ad with merge d93036a...

bors added a commit that referenced this pull request Sep 7, 2017
Debugflag: -Z emit-end-regions

 Skip EndRegion emission by default. Use `-Z emit-end-regions` to reenable it.

The main intent is to fix cases where `EndRegion` emission is believed to be causing excess peak memory pressure.

It may also be a welcome change to people inspecting the MIR output who find the EndRegions to be a distraction.

(In later follow-up PR's I will put in safe-guards against using the current mir-borrowck without enabling `EndRegion` emission. But I wanted this PR to be minimal, in part because we may wish to backport it to the beta channel if we find that it reduces peak memory usage significantly.)
@bors
Copy link
Contributor

bors commented Sep 7, 2017

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

@bors bors merged commit f2892ad into rust-lang:master Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants