Free memory before calling linker #66707
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
|
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 |
|
☔ The latest upstream changes (presumably #66647) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
Zoxc
reviewed
Nov 25, 2019
Zoxc
reviewed
Nov 25, 2019
Zoxc
reviewed
Nov 25, 2019
Zoxc
reviewed
Nov 25, 2019
Zoxc
reviewed
Nov 25, 2019
src/librustc_interface/queries.rs
Outdated
| impl Compiler { | ||
| // This method is different to all the other methods in `Compiler` because | ||
| // it lacks a `Queries` entry. It's also not currently used. It does serve | ||
| // as an example of how `Compiler` can be used, with additional steps added | ||
| // between some passes. And see `rustc_driver::run_compiler` for a more | ||
| // complex example. | ||
| pub fn enter<'c, F, T>(&'c self, f: F) -> Result<T> | ||
| where F: for<'q> FnOnce(&'q Queries<'c>) -> Result<T> | ||
| where F: FnOnce(Queries<'c>) -> Result<T> |
There was a problem hiding this comment.
This closure should take &Queries<'_>. We don't want to give out ownership. The 'c lifetime also isn't needed here. The comment above is also for the wrong function.
|
The goal here is to allow for moving the This PR effectively mirrors #59904 where |
|
I haven't read through the code changes, but I think we're also scheduled to discuss this soon? (rust-lang/compiler-team#175) I guess maybe that's a much broader issue? |
|
@Mark-Simulacrum Yeah, that meeting is about moving the |
|
Generally speaking it's a bit unclear to me -- is this primarily trying to move the Arena into Queries, or is it primarily trying to drop things earlier (i.e., before linking)? The PR title suggests the latter, but your summary comment talks more about the first being the driving force. Can you clarify that perhaps? If this is an optimization (i.e., we're not currently dropping things before linking) then that seems fine (though surprising); we can probably land this if the implementation makes sense to you. If this is about moving the Arena into Queries for other reasons then I might want to look into it a bit more but am largely on board too. |
|
@Mark-Simulacrum It's prerequisite work for moving the |
Zoxc
reviewed
Nov 26, 2019
Zoxc
reviewed
Nov 26, 2019
src/librustc_driver/lib.rs
Outdated
| // Drop GlobalCtxt after starting codegen to free memory | ||
| mem::drop(compiler.global_ctxt()?.take()); | ||
| // Drop GlobalCtxt after starting codegen to free memory | ||
| mem::drop(queries.global_ctxt()?.take()); |
There was a problem hiding this comment.
This can now be removed since it's freed below when we return from the closure anyway.
Zoxc
reviewed
Nov 26, 2019
src/librustc_interface/queries.rs
Outdated
| // Drop GlobalCtxt after starting codegen to free memory. | ||
| mem::drop(self.global_ctxt()?.take()); | ||
| // Drop GlobalCtxt after starting codegen to free memory. | ||
| mem::drop(queries.global_ctxt()?.take()); |
Zoxc
reviewed
Nov 26, 2019
src/librustdoc/core.rs
Outdated
| @@ -343,25 +343,26 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt | |||
| registry: rustc_driver::diagnostics_registry(), | |||
| }; | |||
|
|
|||
| interface::run_compiler_in_existing_thread_pool(config, |compiler| { | |||
| interface::run_compiler_in_existing_thread_pool(config, |compiler| { compiler.enter(|queries| { | |||
There was a problem hiding this comment.
You can drop the first { here.
Zoxc
reviewed
Nov 26, 2019
src/librustc_interface/queries.rs
Outdated
|
|
||
| impl Compiler { | ||
| pub fn enter<F, T>(&self, f: F) -> T | ||
| where F: for<'q> FnOnce(&'q Queries<'_>) -> T |
There was a problem hiding this comment.
You can write this as F: FnOnce(&Queries<'_>) -> T.
Zoxc
reviewed
Nov 26, 2019
src/librustdoc/test.rs
Outdated
| @@ -85,14 +85,14 @@ pub fn run(options: Options) -> i32 { | |||
| let mut test_args = options.test_args.clone(); | |||
| let display_warnings = options.display_warnings; | |||
|
|
|||
| let tests = interface::run_compiler(config, |compiler| -> Result<_, ErrorReported> { | |||
| let lower_to_hir = compiler.lower_to_hir()?; | |||
| let tests = interface::run_compiler(config, |compiler| { compiler.enter(|queries| { | |||
There was a problem hiding this comment.
You can drop the first { here too.
Zoxc
reviewed
Nov 26, 2019
|
This looks good to me with the latest nits fixed. I'm not sure I should approve these changes though since I basically dictated what @cjgillot should do. So I'll just let Mark sign off on the changes. |
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Nov 29, 2019
Handle GlobalCtxt directly from librustc_interface query system This PR constructs the `GlobalCtxt` as a member of the `Queries` in librustc_interface. This simplifies the code to construct it, at the expense of added complexity in the query control flow. This allows to handle the arenas directly from librustc_interface. Based on rust-lang#66707 r? @Zoxc
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Nov 29, 2019
Handle GlobalCtxt directly from librustc_interface query system This PR constructs the `GlobalCtxt` as a member of the `Queries` in librustc_interface. This simplifies the code to construct it, at the expense of added complexity in the query control flow. This allows to handle the arenas directly from librustc_interface. Based on rust-lang#66707 r? @Zoxc
|
☔ The latest upstream changes (presumably #66879) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
The queries in rustc_interfaces are isolated in the
Queriestype. TheQueriesobject constructed to have the smallest possible lifetime, and to drop contents as soon as possible. The information needed by the link phase is extracted through aLinkerobject for later use.r? @Zoxc