Remove tls::with_related_context.#153316
Conversation
This function gets the current `ImplicitCtxt` and checks that its `tcx` matches the passed-in `tcx`. It's an extra bit of sanity checking: when you already have a `tcx`, and you need access to the non-`tcx` parts of `ImplicitCtxt`, check that your `tcx` matches the one in `ImplicitCtxt`. However, it's only used in two places: `start_query` and `current_query_job`. The non-checked alternatives (`with_context`, `with_context_opt`) are used in more places, including some where a `tcx` is available. And things would have to go catastrophically wrong for the check to fail -- e.g. if we somehow end up with multiple `TyCtxt`s. In my opinion it's just an extra case to understand in `tls.rs` that adds little value. This commit removes it. This avoids the need for `tcx` parameters in a couple of places. The commit also adjusts how `start_query` sets up its `ImplicitCtxt` to more closely match how similar functions do it, i.e. with `..icx.clone()` for the unchanged fields.
rustbot
added
A-query-system
|
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remove `tls::with_related_context`.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f3f58ae): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -4.9%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.002s -> 480.332s (0.07%) |
rustbot
removed
the
S-waiting-on-perf
|
Very minor perf wins, |
| let icx = ImplicitCtxt { | ||
| query: Some(job_id), | ||
| query_depth: icx.query_depth + if depth_limit { 1 } else { 0 }, | ||
| ..icx.clone() |
There was a problem hiding this comment.
Suggestion/nit: I'm pretty sure this can just be ..*icx, because all the fields are Copy anyway.
| /// new query job while it executes. | ||
| /// Executes a job by changing the `ImplicitCtxt` to point to the new query job while it executes. | ||
| #[inline(always)] | ||
| pub(crate) fn start_query<'tcx, R>( | ||
| tcx: TyCtxt<'tcx>, | ||
| token: QueryJobId, | ||
| pub(crate) fn start_query<R>( |
There was a problem hiding this comment.
Remark: I've been meaning to rename this function at some point, because I find start_query pretty unhelpful for indicating what it actually does and doesn't do.
5 participants
This function gets the current
ImplicitCtxtand checks that itstcxmatches the passed-intcx. It's an extra bit of sanity checking: when you already have atcx, and you need access to the non-tcxparts ofImplicitCtxt, check that yourtcxmatches the one inImplicitCtxt.However, it's only used in two places:
start_queryandcurrent_query_job. The non-checked alternatives (with_context,with_context_opt) are used in more places, including some where atcxis available. And things would have to go catastrophically wrong for the check to fail -- e.g. if we somehow end up with multipleTyCtxts. In my opinion it's just an extra case to understand intls.rsthat adds little value.This commit removes it. This avoids the need for
tcxparameters in a couple of places. The commit also adjusts howstart_querysets up itsImplicitCtxtto more closely match how similar functions do it, i.e. with..icx.clone()for the unchanged fields.r? @oli-obk