Avoid locking the global context across the after_expansion callback
#107740
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
added
S-waiting-on-review
| @@ -362,8 +364,6 @@ fn run_compiler( | |||
| result | |||
| })?; | |||
|
|
|||
| drop(gctxt); | |||
There was a problem hiding this comment.
This was explicitly added so that GlobalCtxt could be freed before invoking the linker to reduce peak memory usage. Has this been intentionally changed?
There was a problem hiding this comment.
No, I added this just to release the lock before after_analysis: 261bbd7
There was a problem hiding this comment.
I guess the one I added in #56732 was a couple of lines below. I wonder when that went away. It seems like that could easily slip by perf as I assume it doesn't track total memory usage regressions.
There was a problem hiding this comment.
They are tracked by perf, but only really checked by looking at the over-time graphs
There was a problem hiding this comment.
Are you sure? I though it only tracked the rustc process.
There was a problem hiding this comment.
https://perf.rust-lang.org/index.html?start=&end=&kind=percentfromfirst&stat=max-rss shows the max rss changes of all tests over time
There was a problem hiding this comment.
How would this affect memory usage? AFAICT, gctxt is a QueryResult<QueryContext>. QueryContext is just a wrapper for a &'tcx GlobalCtxt. Also, Queries::ongoing_codegen and Queries::linker both call self.global_ctxt(), so the query is repeated there anyway.
There was a problem hiding this comment.
This PR just happened to remind me of an earlier state of rustc_interface where queries.global_ctxt() returned an owned copy which was explictly dropped before the linker was run. The current state of the compiler looks good.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@bors rollup |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107719 (Remove `arena_cache` modifier from `upstream_monomorphizations_for`) - rust-lang#107740 (Avoid locking the global context across the `after_expansion` callback) - rust-lang#107746 (Split fn_ctxt/adjust_fulfillment_errors from fn_ctxt/checks) - rust-lang#107749 (allow quick-edit convenience) - rust-lang#107750 (make more readable) - rust-lang#107755 (remove binder from query constraints) - rust-lang#107756 (miri: fix ICE when running out of address space) - rust-lang#107764 (llvm-16: Use Triple.h from new header location.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
r? @petrochenkov
This was noticed in model-checking/kani#2184 (comment)
This didn't have a perf impact, as it's just an additional 2 or 3 RefCell locks being created.