add depth_limit in QueryVTable to avoid entering a new tcx in layout_of

[SparrowLii]

Fixes #49735
Updates #48685

The layout_of query needs to check whether it overflows the depth limit, and the current implementation needs to create a new ImplicitCtxt inside layout_of. However, start_query will already create a new ImplicitCtxt, so we can check the depth limit in start_query.

We can tell whether we need to check the depth limit simply by whether the return value of to_debug_str of the query is layout_of. But I think adding the depth_limit field in QueryVTable may be more elegant and more scalable.

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[SparrowLii]

cc @nikomatsakis @Zoxc @michaelwoerister

[cjgillot]

This is a great idea!
Running perf because the query engine plumbing is very sensitive.
@bors try @rustc-timer queue

[bors]

⌛ Trying commit d5fe7e466475a917137acb172c22e87e1f212a7e with merge d13747083b5d00d7431b366d24a965382115107f...

[bors]

☀️ Try build successful - checks-actions
Build commit: d13747083b5d00d7431b366d24a965382115107f (d13747083b5d00d7431b366d24a965382115107f)

[cjgillot]

@rust-timer build d13747083b5d00d7431b366d24a965382115107f

[rust-timer]

Queued d13747083b5d00d7431b366d24a965382115107f with parent e1b28cd, future comparison URL.

[rust-timer]

Finished benchmarking commit (d13747083b5d00d7431b366d24a965382115107f): comparison url.

Instruction count

• Primary benchmarks: ❌ relevant regression found
• Secondary benchmarks: ❌ relevant regressions found

	mean1	max	count2
Regressions ❌ (primary)	0.2%	0.2%	1
Regressions ❌ (secondary)	0.4%	0.5%	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	0.2%	1

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: ✅ relevant improvements found

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	-3.4%	4
All ❌✅ (primary)	-	-	0

Cycles

Results

• Primary benchmarks: ✅ relevant improvement found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.8%	-3.8%	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.8%	-3.8%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[cjgillot]

A single benchmark has a minor regression.
@bors r+

[bors]

📌 Commit d5fe7e466475a917137acb172c22e87e1f212a7e has been approved by cjgillot

It is now in the queue for this repository.

[SparrowLii]

Thanks for reviewing!

[bors]

⌛ Testing commit d5fe7e466475a917137acb172c22e87e1f212a7e with merge 125eb92bed75ab95ad00039ed2c3809fd73449b3...

[bors]

💔 Test failed - checks-actions

[SparrowLii]

Rebased on top of #100723 to fix the build failure

[cjgillot]

This used to be a fatal error message. Could you make it so?
Panics are considered ICEs, and ask for a bug report. This is not the case.

[SparrowLii]

Yea, I added depth_limit_error func in QueryContext so we can keep #![deny(rustc::diagnostic_outside_of_impl)] in rustc_query_impl

[bors]

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

[SparrowLii]

rebased again

[cjgillot]

@bors r+

[bors]

📌 Commit cbc6bd2 has been approved by cjgillot

It is now in the queue for this repository.

[nnethercote]

This currently has rollup=never, but the perf effects were negligible. Is it suitable for rolling up? (The queue is long right now.)

[bors]

⌛ Testing commit cbc6bd2 with merge ae14a41981cd265662b36257f1067b7fe664fa57...

[bors]

💥 Test timed out

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[SparrowLii]

There seems to be something wrong with CI itself. Can it try again？

[cjgillot]

@bors retry

[bors]

⌛ Testing commit cbc6bd2 with merge cfb5ae2...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing cfb5ae2 to master...

[rust-timer]

Finished benchmarking commit (cfb5ae2): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	0.5%	[0.3%, 0.7%]	8
Regressions ❌ (secondary)	0.7%	[0.3%, 1.3%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.3%, 0.7%]	8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-2.2%, -1.5%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	4.0%	[4.0%, 4.0%]	1
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.0%	[4.0%, 4.0%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[rylev]

Most of the regressions are happening in html5ever-0.26.0 and deeply-nested-multi which have been noisy lately. The regressions are small enough that it's likely that we're seeing that noise here too. Subsequent changes show improvements of the same magnitude reversing the regressions here.

However, there are some regressions that seem like they might be real, and they are all in doc profile test cases. The common query across the potentially real regressions is the build_impl query. This change seems like strictly less work, so I'm confused why this might be.

The cachegrind diff doesn't show anythign immeadiately obvious:

1,606,140  ???:_rjem_je_arena_cache_bin_fill_small
1,534,640  ???:<alloc::vec::Vec<rustdoc::clean::types::GenericArg>>::into_boxed_slice
  781,591  ???:_rjem_je_malloc_default
  470,388  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::plumbing::QueryCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, &[rustc_ast::ast::Attribute]>>
 -373,310  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms
  301,962  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::explicit_predicates_of, rustc_query_impl::plumbing::QueryCtxt>
  296,379  ???:<rustdoc::clean::types::GenericArg as alloc::slice::hack::ConvertVec>::to_vec::<alloc::alloc::Global>
 -287,480  ???:<alloc::boxed::Box<[rustdoc::clean::types::GenericArg]> as core::clone::Clone>::clone
  280,160  /build/glibc-eX1tMB/glibc-2.31/nptl/../nptl/pthread_mutex_trylock.c:pthread_mutex_trylock
  275,582  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::generics_of, rustc_query_impl::plumbing::QueryCtxt>
  271,458  ???:_rjem_je_tcache_alloc_small_hard
  255,130  ???:_rjem_je_tcache_bin_flush_stashed
  238,323  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::type_of, rustc_query_impl::plumbing::QueryCtxt>
  234,287  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::visibility, rustc_query_impl::plumbing::QueryCtxt>
  233,837  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::impl_trait_ref, rustc_query_impl::plumbing::QueryCtxt>
  204,215  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::plumbing::QueryCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, bool>>
  179,493  ???:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::def_ident_span, rustc_query_impl::plumbing::QueryCtxt>
  172,787  ???:arena_choose
  156,605  ???:rustdoc::clean::ut

@SparrowLii @cjgillot @nnethercote any ideas or shall we just accept this small hit to docs?

[nnethercote]

The _rjem_* ones are jemalloc, i.e. allocation, which makes sense in combination with the into_boxed_slice. Though I don't see into_boxed_slice in the diff...