Support unstable moves via stable in unstable items #95956

yaahc · 2022-04-12T01:16:25Z

part of https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/moving.20items.20to.20core.20unstably and a blocker of #90328.

The libs-api team needs the ability to move an already stable item to a new location unstably, in this case for Error in core. Otherwise these changes are insta-stable making them much harder to merge.

This PR attempts to solve the problem by checking the stability of path segments as well as the last item in the path itself, which is currently the only thing checked.

rust-highfive · 2022-04-12T01:16:28Z

r? @cjgillot

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

src/test/ui/intrinsics/const-eval-select-bad.rs

src/test/ui/lint/lint-stability.rs

library/std/src/lib.rs

library/std/src/panic.rs

library/core/src/unicode/mod.rs

yaahc · 2022-04-26T20:09:09Z

This is now also a T-libs-api PR because of the core::unicode stabilization :/

yaahc · 2022-04-26T21:28:41Z

Also blocked on rust-lang/stdarch#1303

@rustbot blocked

library/std/src/lib.rs

yaahc · 2022-04-29T17:34:15Z

This should be ready now that rust-lang/stdarch#1303 has merged.

@rustbot ready

bors · 2022-07-14T13:42:14Z

⌛ Testing commit 3e2c5b5 with merge 24699bc...

bors · 2022-07-14T16:23:00Z

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

rust-timer · 2022-07-14T17:38:44Z

Finished benchmarking commit (24699bc): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	0.5%	0.9%	13
Regressions 😿 (secondary)	0.6%	1.2%	10
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.5%	0.9%	13

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	1.5%	1.5%	1
Regressions 😿 (secondary)	7.3%	9.8%	2
Improvements 🎉 (primary)	-5.7%	-5.7%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.1%	-5.7%	2

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	3.6%	3.6%	1
Regressions 😿 (secondary)	3.3%	4.4%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.6%	3.6%	1

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

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Fixes rust-lang#99286 PR rust-lang#95956 accidentally made these intrinsics unstable when accessed through the unstable path segment 'std::intrinsics'

Mark stabilized intrinsics with `rustc_allowed_through_unstable_modules` Fixes rust-lang#99286 PR rust-lang#95956 accidentally made these intrinsics unstable when accessed through the unstable path segment 'std::intrinsics'

Support unstable moves via stable in unstable items part of https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/moving.20items.20to.20core.20unstably and a blocker of rust-lang#90328. The libs-api team needs the ability to move an already stable item to a new location unstably, in this case for Error in core. Otherwise these changes are insta-stable making them much harder to merge. This PR attempts to solve the problem by checking the stability of path segments as well as the last item in the path itself, which is currently the only thing checked.

rylev · 2022-07-19T16:06:51Z

@yaahc so this PR led to a performance regression - I see that a perf run was performed (accidentally) before this was merged, but it wasn't looked at. Is there a reason that the performance difference wasn't taken into account? In either case, we should at least address the performance regression with a justification if one is warranted.

The change seems to be localized to incremental compilation scenarios. It seems queries like incr_comp_encode_dep_graph are being hit more often. Is this simply because of the additional pass that's happening or is there some other explanation I don't see?

yaahc · 2022-07-19T20:33:16Z

@yaahc so this PR led to a performance regression - I see that a perf run was performed (accidentally) before this was merged, but it wasn't looked at. Is there a reason that the performance difference wasn't taken into account?

only because I hadn't intended to run it in the first place, I just didn't pay attention to it.

In either case, we should at least address the performance regression with a justification if one is warranted.

The change seems to be localized to incremental compilation scenarios. It seems queries like incr_comp_encode_dep_graph are being hit more often. Is this simply because of the additional pass that's happening or is there some other explanation I don't see?

I don't have a solid explanation for why that's happening, I don't have much experience with interpreting perf reports for the compiler. Where are you seeing that incr_comp_encode_dep_graph is being hit more often?

From the impl itself I'd expect that the main difference is that we hit the stability check function much more often because of: https://github.com/rust-lang/rust/pull/95956/files#diff-5f57ad10e1bdde3d046b258d390fd2ecc6f1511158aa130cebb72093da16ef29R851, but I don't know how that relates to incremental compilation at all.

rylev · 2022-07-20T16:36:14Z

You can see differences in query perf on this page. You can see there that incr_comp_encode_dep_graph was executed more often.

I'm also unsure, as I don't see anything that would cause encoding the dep graph more often, but I'm really not familiar enough with this code to say for sure.

The regressions are sort of right on the line between being small enough to shrug off and big enough to really care about investigating. I'll leave it to you on what to do next. Feel free to reach out if you want to chat through things.

nnethercote · 2022-07-21T02:42:20Z

I ran a Cachegrind diff. The results absolutely match up with what @rylev said about the incremental dep graph.

--------------------------------------------------------------------------------
Ir
--------------------------------------------------------------------------------
72,523,795 (100.0%)  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir                  file:function
--------------------------------------------------------------------------------
1,988,457 ( 2.74%)  compiler/rustc_query_system/src/dep_graph/graph.rs:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
1,759,692 ( 2.43%)  library/core/src/num/uint_macros.rs:<rustc_data_structures::stable_hasher::StableHasher>::finish::<rustc_data_structures::fingerprint::Fingerprint>
1,537,026 ( 2.12%)  compiler/rustc_serialize/src/leb128.rs:<rustc_query_system::dep_graph::serialized::SerializedDepGraph<rustc_middle::dep_graph::dep_node::DepKind> as rustc_serialize::serialize::Decodable<rustc_serialize::opaque::MemDecoder>>::decode
1,455,664 ( 2.01%)  compiler/rustc_serialize/src/leb128.rs:<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_query_impl::on_disk_cache::AbsoluteBytePos) as rustc_serialize::serialize::Decodable<rustc_serialize::opaque::MemDecoder>>::decode
1,368,740 ( 1.89%)  compiler/rustc_middle/src/dep_graph/mod.rs:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure#0}>
1,260,818 ( 1.74%)  compiler/rustc_query_system/src/query/plumbing.rs:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::plumbing::QueryCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, core::option::Option<rustc_attr::builtin::Stability>>>
1,225,791 ( 1.69%)  github.com-1ecc6299db9ec823/hashbrown-0.12.0/src/raw/mod.rs:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure#0}>
1,155,286 ( 1.59%)  compiler/rustc_data_structures/src/sip128.rs:<rustc_data_structures::stable_hasher::StableHasher>::finish::<rustc_data_structures::fingerprint::Fingerprint>
1,091,722 ( 1.51%)  github.com-1ecc6299db9ec823/hashbrown-0.12.0/src/raw/mod.rs:rustc_query_system::query::plumbing::try_get_cached::<rustc_middle::ty::context::TyCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, core::option::Option<rustc_attr::builtin::Stability>>, core::option::Option<rustc_attr::builtin::Stability>, rustc_middle::ty::query::copy<core::option::Option<rustc_attr::builtin::Stability>>>
1,015,419 ( 1.40%)  compiler/rustc_query_system/src/query/plumbing.rs:rustc_query_system::query::plumbing::try_get_cached::<rustc_middle::ty::context::TyCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, core::option::Option<rustc_attr::builtin::Stability>>, core::option::Option<rustc_attr::builtin::Stability>, rustc_middle::ty::query::copy<core::option::Option<rustc_attr::builtin::Stability>>>
  995,121 ( 1.37%)  compiler/rustc_serialize/src/leb128.rs:<[rustc_query_system::dep_graph::graph::DepNodeIndex] as rustc_serialize::serialize::Encodable<rustc_serialize::opaque::FileEncoder>>::encode
  969,062 ( 1.34%)  github.com-1ecc6299db9ec823/smallvec-1.8.1/src/lib.rs:<smallvec::SmallVec<[rustc_query_system::dep_graph::graph::DepNodeIndex; 8]> as core::iter::traits::collect::Extend<rustc_query_system::dep_graph::graph::DepNodeIndex>>::extend::<core::iter::adapters::map::Map<core::slice::iter::Iter<rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex>, <rustc_query_system::dep_graph::graph::CurrentDepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::promote_node_ad_deps_to_current::{closure#0}>>
  958,608 ( 1.32%)  compiler/rustc_serialize/src/leb128.rs:<[(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_query_impl::on_disk_cache::AbsoluteBytePos)] as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
  944,089 ( 1.30%)  compiler/rustc_query_system/src/dep_graph/serialized.rs:<rustc_query_system::dep_graph::serialized::GraphEncoder<rustc_middle::dep_graph::dep_node::DepKind>>::send
  923,416 ( 1.27%)  compiler/rustc_query_system/src/query/plumbing.rs:rustc_query_system::query::plumbing::get_query::<rustc_query_impl::queries::lookup_stability, rustc_query_impl::plumbing::QueryCtxt>
  923,104 ( 1.27%)  compiler/rustc_query_impl/src/on_disk_cache.rs:<rustc_query_impl::on_disk_cache::OnDiskCache>::try_load_query_result::<core::option::Option<rustc_attr::builtin::Stability>>
  826,887 ( 1.14%)  compiler/rustc_middle/src/ty/query.rs:<rustc_passes::stability::Checker as rustc_hir::intravisit::Visitor>::visit_path
  816,784 ( 1.13%)  compiler/rustc_query_system/src/query/plumbing.rs:rustc_query_system::query::plumbing::try_load_from_disk_and_cache_in_memory::<rustc_query_impl::plumbing::QueryCtxt, rustc_span::def_id::DefId, core::option::Option<rustc_attr::builtin::Stability>>
  745,584 ( 1.03%)  github.com-1ecc6299db9ec823/hashbrown-0.12.0/src/raw/mod.rs:<hashbrown::raw::RawTable<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_query_impl::on_disk_cache::AbsoluteBytePos)>>::insert::<hashbrown::map::make_hasher<rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_query_impl::on_disk_cache::AbsoluteBytePos, core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure#0}>

Correctly handle path stability for 'use tree' items PR rust-lang#95956 started checking the stability of path segments. However, this was not applied to 'use tree' items (e.g. 'use some::path::{ItemOne, ItemTwo}') due to the way that we desugar these items in HIR lowering. This PR modifies 'use tree' lowering to preserve resolution information, which is needed by stability checking.

rust-highfive assigned cjgillot Apr 12, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2022

yaahc force-pushed the stable-in-unstable branch from f078c1c to fc7dd28 Compare April 12, 2022 01:18

This comment has been minimized.

Sign in to view

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2022

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

yaahc force-pushed the stable-in-unstable branch 2 times, most recently from 9727750 to e012a80 Compare April 26, 2022 16:56