fix non_blanket_impls iteration order #89016

lcnr · 2021-09-16T16:36:20Z

We sometimes iterate over all non_blanket_impls, not sure if this is observable outside
of error messages (i.e. as incremental bugs). This should fix the underlying issue of #86986.

second attempt of #88718

r? @nikomatsakis

lcnr · 2021-09-16T16:42:28Z

@bors try @rust-timer queue

rust-timer · 2021-09-16T16:42:29Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-09-16T16:42:36Z

⌛ Trying commit 92bbb2f7811fe8c9b6148b38597960a601f768ee with merge 91bb5f03171b819eba4a4463ce46e49de79a7e3b...

bors · 2021-09-16T18:08:35Z

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

bors · 2021-09-16T18:08:46Z

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

rust-timer · 2021-09-16T18:09:09Z

Queued 91bb5f03171b819eba4a4463ce46e49de79a7e3b with parent 2b5ddf3, future comparison URL.

rust-timer · 2021-09-16T21:22:13Z

Finished benchmarking commit (91bb5f03171b819eba4a4463ce46e49de79a7e3b): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Moderate improvement in instruction counts (up to -1.8% on full builds of inflate)
Moderate regression in instruction counts (up to 1.8% on full builds of externs)

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Mark-Simulacrum · 2021-09-16T21:28:12Z

compiler/rustc_middle/src/ty/trait_def.rs

 pub struct TraitImpls {
    blanket_impls: Vec<DefId>,
    /// Impls indexed by their simplified self type, for fast lookup.
-    non_blanket_impls: FxHashMap<fast_reject::SimplifiedType, Vec<DefId>>,
+    non_blanket_impls: FxIndexMap<fast_reject::SimplifiedType, Vec<DefId>>,


Hm, is this enough?

It looks like insertion into non_blanket_impls happens in a loop which is a result of tcx.crates(()) which isn't stable across compilation sessions I'd expect (though I could be wrong).

The way crates(()) is computed, it looks like it is always (1..=num_crates).map(CrateNum) (0 is reserved for the current crate and does not have any data) and it I would assume the crates get added to the CStore in a deterministic order.

We do have stable_crate_id which orders crates independently of their cnum. I could be that CrateNums are not deterministically assigned to their actual crate data, though it seems quite buggy to me if this doesn't invalidate query caches in that case. e.g. trait_impls_of_provider just uses the order of crates(()) which influences the way winnowing works for example.

A bit out of my depth here, maybe @rust-lang/wg-incr-comp can help and clarify here 😅 ❤️

Yeah, we probably do end up with an ordering that is mostly stable from run to run (presuming no shifts in code or other sources), and I think incremental will track the query fine. My guess is that we could avoid marking the trait query as red if the crate load order changes (e.g., because the first code using that crate is moved?) if this used a stable crate ID, but it may not be worth it. Ultimately this PR does seem like it makes things strictly better here :)

I think that crate ordering is indeed deterministic, since it isn't something that we discover dynamically? But indeed I bet @michaelwoerister or @petrochenkov would be certain.

Crate are assigned their CrateNum in the order in which they are required by the resolver. As the resolver is not incremental, this order is deterministic but not stable. On the other hand, it is not obvious at all.
Incremental computes a hash for crates(()) based on their order in the vector and their stable_crate_id. We could make it independent on the order of crates by having the crates query sort its result by increasing stable_crate_id.

I think that parts of the code might rely on tcx.crates(()) being sorted by CrateNum, for better or worse. I would not recommend trying to change that as part of this PR.

nikomatsakis · 2021-09-17T18:25:28Z

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

-            crates.sort();
-
-            for &cnum in crates.iter() {
+            for &cnum in tcx.crates(()) {


are these changes just "cleanup" or is there some substantive change in the order in which things are done here?

just cleanup, the only change of behavior is the second commit here.

crates(()) already returns the cnums in order, so the code i deleted in the other commits should just be noops

nikomatsakis · 2021-09-17T20:03:48Z

r=me, but I'm going to make this

r? @michaelwoerister

since the primary question seems to have to do with incremental.

cjgillot · 2021-09-17T21:43:14Z

compiler/rustc_metadata/src/dependency_format.rs

-        .map(|cnum| {
-            if tcx.dep_kind(CrateNum::new(cnum)) == CrateDepKind::Explicit {
+    let mut ret = tcx.crates(()).iter()
+        .map(|&cnum| {


Do we need to skip the leading CrateNum(0)?

🤷 CrateNum(0) is the local crate so I don't think we need to do any linking for that.

I think that explicitly dealing with the local crate and only iterating over all external ones is probably better than specialcasing the first item of the crates iter. But that's a fairly uninformed opinion

I don't understand how that code works in the first place? last_crate is tcx.crates(()).len() and then it creates a CrateNum(last_crate)? That should not be a valid CrateNum, right?

Oh, tcx.crates(()) only returns upstream CrateNums. That is far from obvious. I think the query should be renamed to upstream_crate_nums or something. The same goes for CStore::crates_untracked. Would you be up for doing that, @lcnr? (but maybe in a separate PR)

extern_crates maybe? 🤔 though i prefer doing that in a separate PR, want to merge this as it is blocking some further work

I've personally always used the term "upstream" (e.g. see

rust/compiler/rustc_middle/src/query/mod.rs

Line 1264 in 15d9ba0

query upstream_monomorphizations(_: ()) -> DefIdMap<FxHashMap<SubstsRef<'tcx>, CrateNum>> {

). "Extern" often refers to something non-Rust. Although extern crate foo suggests otherwise :).

Anyway, I'm fine with doing this in another PR. You can r=me on this one.

upstream is also fine 😆 was thinking of extern crate something ^^

michaelwoerister · 2021-09-22T08:36:25Z

I'll take a look tomorrow.

michaelwoerister · 2021-09-23T11:26:13Z

compiler/rustc_middle/src/ich/hcx.rs

-        }
-
-        blanket_impls.hash_stable(hcx, hasher);
-    }


This actually looks like it is unsound, right @Aaron1011 & @cjgillot? If the order is observable, we must not "stabilize" it via sorting.

michaelwoerister · 2021-09-23T11:33:17Z

This looks good to me. If anything it makes the code more correct (see #89016 (comment)).

I would also recommend renaming the crates query to upstream_crate_nums (since it does not include LOCAL_CRATE) and document that its result is expected to be sorted by the numerical CrateNum value.

lcnr · 2021-09-23T14:27:57Z

@bors r=nikomatsakis,michaelwoerister rollup=never

bors · 2021-09-23T14:27:59Z

📌 Commit 01bcddb has been approved by nikomatsakis,michaelwoerister

bors · 2021-09-23T15:44:57Z

⌛ Testing commit 01bcddb with merge bf64232...

bors · 2021-09-23T18:51:32Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis,michaelwoerister
Pushing bf64232 to master...

rust-highfive · 2021-09-23T18:52:02Z

📣 Toolstate changed by #89016!

Tested on commit bf64232.
Direct link to PR: #89016

🎉 rls on linux: test-fail → test-pass (cc @Xanewok).

@Xanewok

Tested on commit rust-lang/rust@bf64232. Direct link to PR: <rust-lang/rust#89016> 🎉 rls on linux: test-fail → test-pass (cc @Xanewok).

rust-timer · 2021-09-23T22:13:07Z

Finished benchmarking commit (bf64232): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -2.6% on full builds of keccak)
Moderate regression in instruction counts (up to 2.8% on incr-full builds of ctfe-stress-4)

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

rylev · 2021-09-28T17:19:40Z

The improvements outweigh the regressions with the only regressions being in historically noisy benchmarks. As such I'm going to mark this as triaged.

@rustbot label +perf-regression-triaged

rust-highfive assigned nikomatsakis Sep 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 16, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 16, 2021

Mark-Simulacrum reviewed Sep 16, 2021

View reviewed changes

nikomatsakis reviewed Sep 17, 2021

View reviewed changes

rust-highfive assigned michaelwoerister and unassigned nikomatsakis Sep 17, 2021

cjgillot reviewed Sep 17, 2021

View reviewed changes

lcnr force-pushed the non_blanket_impls branch from 2fe7145 to 7c076fe Compare September 20, 2021 14:21

This comment has been minimized.

Sign in to view

lcnr force-pushed the non_blanket_impls branch from 7c076fe to a8b6a91 Compare September 20, 2021 17:06

lcnr added 3 commits September 21, 2021 08:24

rename

ca2c55d

use indexmap instead of hashmap

5b9cc20

crates is already deterministic

01bcddb

lcnr force-pushed the non_blanket_impls branch from a8b6a91 to 01bcddb Compare September 21, 2021 06:24

michaelwoerister reviewed Sep 23, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2021

bors mentioned this pull request Sep 23, 2021

Revise never type fallback algorithm #88804

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2021

bors merged commit bf64232 into rust-lang:master Sep 23, 2021

rustbot added this to the 1.57.0 milestone Sep 23, 2021

lcnr mentioned this pull request Sep 24, 2021

extend simplify_type #86986

Merged

lcnr deleted the non_blanket_impls branch September 24, 2021 19:40

Mark-Simulacrum mentioned this pull request Sep 27, 2021

Use ZST for fmt unsafety #89139

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix non_blanket_impls iteration order #89016

fix non_blanket_impls iteration order #89016

lcnr commented Sep 16, 2021

lcnr commented Sep 16, 2021

rust-timer commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

rust-timer commented Sep 16, 2021

rust-timer commented Sep 16, 2021

Mark-Simulacrum Sep 16, 2021

lcnr Sep 17, 2021

Mark-Simulacrum Sep 17, 2021

nikomatsakis Sep 17, 2021

cjgillot Sep 17, 2021

michaelwoerister Sep 23, 2021

nikomatsakis Sep 17, 2021

lcnr Sep 17, 2021

nikomatsakis commented Sep 17, 2021

cjgillot Sep 17, 2021

lcnr Sep 18, 2021

michaelwoerister Sep 23, 2021

michaelwoerister Sep 23, 2021

lcnr Sep 23, 2021

michaelwoerister Sep 23, 2021

lcnr Sep 23, 2021

This comment has been minimized.

michaelwoerister commented Sep 22, 2021

michaelwoerister Sep 23, 2021

michaelwoerister commented Sep 23, 2021

lcnr commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

rust-highfive commented Sep 23, 2021

rust-timer commented Sep 23, 2021

rylev commented Sep 28, 2021

fix non_blanket_impls iteration order #89016

fix non_blanket_impls iteration order #89016

Conversation

lcnr commented Sep 16, 2021

lcnr commented Sep 16, 2021

rust-timer commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

rust-timer commented Sep 16, 2021

rust-timer commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

michaelwoerister commented Sep 22, 2021

Choose a reason for hiding this comment

michaelwoerister commented Sep 23, 2021

lcnr commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

rust-highfive commented Sep 23, 2021

rust-timer commented Sep 23, 2021

rylev commented Sep 28, 2021