Store CtxtInterners for local values in AllArenas #57214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
Zoxc
force-pushed
the
no-local-interners
branch
2 times, most recently
from
9298bd8 to
e261ca0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Zoxc
force-pushed
the
no-local-interners
branch
from
e585dbb to
0bb05f4
Compare
This comment has been minimized.
This comment has been minimized.
Zoxc
force-pushed
the
no-local-interners
branch
from
0bb05f4 to
e4c68a2
Compare
|
@bors try |
|
⌛ Trying commit e4c68a2436aebe43d3c61745c393d7c2180e2ace with merge 005d7a2944755845526457d6176ace8b35934ef3... |
|
☀️ Test successful - status-travis |
|
@rust-timer build 005d7a2944755845526457d6176ace8b35934ef3 |
|
Success: Queued 005d7a2944755845526457d6176ace8b35934ef3 with parent 953a9cf, comparison URL. |
|
Finished benchmarking try commit 005d7a2944755845526457d6176ace8b35934ef3 |
|
It doesn't seem like local interners are effective in keeping memory usage down. Is there some other reason to have them? cc @rust-lang/compiler |
|
@bors try |
|
⌛ Trying commit 1936a000fd80a8011738ddb23a14deba8a61564c with merge 2eb7307a44bc76a18eeb0c4d2947492b2be4cbf2... |
|
The job 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 |
|
☀️ Test successful - status-travis |
|
@rust-timer build 2eb7307a44bc76a18eeb0c4d2947492b2be4cbf2 |
|
Success: Queued 2eb7307a44bc76a18eeb0c4d2947492b2be4cbf2 with parent aeed63b, comparison URL. |
|
Finished benchmarking try commit 2eb7307a44bc76a18eeb0c4d2947492b2be4cbf2 |
nikomatsakis
added
I-nominated
T-compiler
|
Nominating for discussion at a @rust-lang/compiler meeting -- afaik, the whole For a decision this major, though, it seems like it would behoove us to gather more data than just perf. @Zoxc do you think you could measure bootstrap memory usage for stage2 or something like that? i'm not sure what's the easiest way to do that -- perhaps |
|
I measured the maximum size of a local arena in |
|
With some more options enabled (debug-assertions, debuginfo, experimental-parallel-queries) peak memory usage rises to 4.25 GB giving a 3% regression. |
|
💔 Test failed - checks-travis |
|
The job 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 |
bors
added
S-waiting-on-review
|
cc #55861 |
|
@bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 66a376e with merge 6220254a46ca08f33bfd0e2182fd10e4abb14a0d... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 3, 2019
Store CtxtInterners for local values in AllArenas r? @eddyb
|
☀️ Test successful - checks-travis, status-appveyor |
bors
added a commit
that referenced
this pull request
Jun 14, 2019
Unify all uses of 'gcx and 'tcx. This is made possible by @Zoxc landing #57214 (see #57214 (comment) for the decision). A bit of context for the approach: just like #61722, this is *not* how I originally intended to go about this, but @Zoxc and my own experimentation independently resulted in the same conclusion: The interim alias `type TyCx<'tcx> = TyCtxt<'tcx, 'tcx>;` attempt required more work (adding `use`s), even only for handling the `TyCtxt<'tcx, 'tcx>` case and not the general `TyCtxt<'gcx, 'tcx>` one. What this PR is based on is the realization that `'gcx` is a special-enough name that it can be replaced, without caring for context, with `'tcx`, and then repetitions of the name `'tcx` be compacted away. After that, only a small number of error categories remained, each category easily dealt with with either more mass replacements (e.g. `TyCtxt<'tcx, '_>` -> `TyCtxt<'tcx>`) or by hand. For the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735 and #61722, and like the latter, there was also a weird bug to work around. It should be reviewed separately, and dropped if unwanted (in this PR it's pretty significant). cc @rust-lang/compiler r? @nikomatsakis
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 4, 2019
Use a single CtxtInterners Builds on rust-lang#57214 r? @eddyb
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jul 4, 2019
Use a single CtxtInterners Builds on rust-lang#57214 r? @eddyb
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jul 4, 2019
Use a single CtxtInterners Builds on rust-lang#57214 r? @eddyb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @eddyb