Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix non_blanket_impls iteration order #89016

Merged
merged 3 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions compiler/rustc_metadata/src/dependency_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ fn attempt_static(tcx: TyCtxt<'_>) -> Option<DependencyList> {
let all_crates_available_as_rlib = tcx
.crates(())
.iter()
.cloned()
.copied()
.filter_map(|cnum| {
if tcx.dep_kind(cnum).macros_only() {
return None;
Expand All @@ -291,10 +291,11 @@ fn attempt_static(tcx: TyCtxt<'_>) -> Option<DependencyList> {

// All crates are available in an rlib format, so we're just going to link
// everything in explicitly so long as it's actually required.
let last_crate = tcx.crates(()).len();
let mut ret = (1..last_crate + 1)
.map(|cnum| {
if tcx.dep_kind(CrateNum::new(cnum)) == CrateDepKind::Explicit {
let mut ret = tcx
.crates(())
.iter()
.map(|&cnum| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if tcx.dep_kind(cnum) == CrateDepKind::Explicit {
Linkage::Static
} else {
Linkage::NotLinked
Expand Down
54 changes: 20 additions & 34 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,17 +304,7 @@ pub fn provide(providers: &mut Providers) {
// traversal, but not globally minimal across all crates.
let bfs_queue = &mut VecDeque::new();

// Preferring shortest paths alone does not guarantee a
// deterministic result; so sort by crate num to avoid
// hashtable iteration non-determinism. This only makes
// things as deterministic as crate-nums assignment is,
// which is to say, its not deterministic in general. But
// we believe that libstd is consistently assigned crate
// num 1, so it should be enough to resolve #46112.
let mut crates: Vec<CrateNum> = (*tcx.crates(())).to_owned();
crates.sort();

for &cnum in crates.iter() {
for &cnum in tcx.crates(()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Ignore crates without a corresponding local `extern crate` item.
if tcx.missing_extern_crate_item(cnum) {
continue;
Expand All @@ -323,35 +313,31 @@ pub fn provide(providers: &mut Providers) {
bfs_queue.push_back(DefId { krate: cnum, index: CRATE_DEF_INDEX });
}

// (restrict scope of mutable-borrow of `visible_parent_map`)
{
let visible_parent_map = &mut visible_parent_map;
let mut add_child = |bfs_queue: &mut VecDeque<_>, child: &Export, parent: DefId| {
if child.vis != ty::Visibility::Public {
return;
}
let mut add_child = |bfs_queue: &mut VecDeque<_>, child: &Export, parent: DefId| {
if child.vis != ty::Visibility::Public {
return;
}

if let Some(child) = child.res.opt_def_id() {
match visible_parent_map.entry(child) {
Entry::Occupied(mut entry) => {
// If `child` is defined in crate `cnum`, ensure
// that it is mapped to a parent in `cnum`.
if child.is_local() && entry.get().is_local() {
entry.insert(parent);
}
}
Entry::Vacant(entry) => {
if let Some(child) = child.res.opt_def_id() {
match visible_parent_map.entry(child) {
Entry::Occupied(mut entry) => {
// If `child` is defined in crate `cnum`, ensure
// that it is mapped to a parent in `cnum`.
if child.is_local() && entry.get().is_local() {
entry.insert(parent);
bfs_queue.push_back(child);
}
}
Entry::Vacant(entry) => {
entry.insert(parent);
bfs_queue.push_back(child);
}
}
};
}
};

while let Some(def) = bfs_queue.pop_front() {
for child in tcx.item_children(def).iter() {
add_child(bfs_queue, child, def);
}
while let Some(def) = bfs_queue.pop_front() {
for child in tcx.item_children(def).iter() {
add_child(bfs_queue, child, def);
}
}

Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1708,9 +1708,10 @@ impl EncodeContext<'a, 'tcx> {

fn encode_crate_deps(&mut self) -> Lazy<[CrateDep]> {
empty_proc_macro!(self);
let crates = self.tcx.crates(());

let mut deps = crates
let deps = self
.tcx
.crates(())
.iter()
.map(|&cnum| {
let dep = CrateDep {
Expand All @@ -1724,8 +1725,6 @@ impl EncodeContext<'a, 'tcx> {
})
.collect::<Vec<_>>();

deps.sort_by_key(|&(cnum, _)| cnum);

{
// Sanity-check the crate numbers
let mut expected_cnum = 1;
Expand Down
43 changes: 2 additions & 41 deletions compiler/rustc_middle/src/ich/hcx.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use crate::ich;
use crate::middle::cstore::CrateStore;
use crate::ty::{fast_reject, TyCtxt};
use crate::ty::TyCtxt;

use rustc_ast as ast;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::sync::Lrc;
use rustc_hir as hir;
Expand All @@ -14,9 +14,6 @@ use rustc_span::source_map::SourceMap;
use rustc_span::symbol::Symbol;
use rustc_span::{BytePos, CachingSourceMapView, SourceFile, Span, SpanData};

use smallvec::SmallVec;
use std::cmp::Ord;

fn compute_ignored_attr_names() -> FxHashSet<Symbol> {
debug_assert!(!ich::IGNORED_ATTRIBUTES.is_empty());
ich::IGNORED_ATTRIBUTES.iter().copied().collect()
Expand Down Expand Up @@ -241,39 +238,3 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> {
}

impl rustc_session::HashStableContext for StableHashingContext<'a> {}

pub fn hash_stable_trait_impls<'a>(
hcx: &mut StableHashingContext<'a>,
hasher: &mut StableHasher,
blanket_impls: &[DefId],
non_blanket_impls: &FxHashMap<fast_reject::SimplifiedType, Vec<DefId>>,
) {
{
let mut blanket_impls: SmallVec<[_; 8]> =
blanket_impls.iter().map(|&def_id| hcx.def_path_hash(def_id)).collect();

if blanket_impls.len() > 1 {
blanket_impls.sort_unstable();
}

blanket_impls.hash_stable(hcx, hasher);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


{
let mut keys: SmallVec<[_; 8]> =
non_blanket_impls.keys().map(|k| (k, k.map_def(|d| hcx.def_path_hash(d)))).collect();
keys.sort_unstable_by(|&(_, ref k1), &(_, ref k2)| k1.cmp(k2));
keys.len().hash_stable(hcx, hasher);
for (key, ref stable_key) in keys {
stable_key.hash_stable(hcx, hasher);
let mut impls: SmallVec<[_; 8]> =
non_blanket_impls[key].iter().map(|&impl_id| hcx.def_path_hash(impl_id)).collect();

if impls.len() > 1 {
impls.sort_unstable();
}

impls.hash_stable(hcx, hasher);
}
}
}
4 changes: 1 addition & 3 deletions compiler/rustc_middle/src/ich/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
//! ICH - Incremental Compilation Hash

pub use self::hcx::{
hash_stable_trait_impls, NodeIdHashingMode, StableHashingContext, StableHashingContextProvider,
};
pub use self::hcx::{NodeIdHashingMode, StableHashingContext, StableHashingContextProvider};
use rustc_span::symbol::{sym, Symbol};

mod hcx;
Expand Down
18 changes: 4 additions & 14 deletions compiler/rustc_middle/src/traits/specialization_graph.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use crate::ich::{self, StableHashingContext};
use crate::ty::fast_reject::SimplifiedType;
use crate::ty::fold::TypeFoldable;
use crate::ty::{self, TyCtxt};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::fx::FxIndexMap;
use rustc_errors::ErrorReported;
use rustc_hir::def_id::{DefId, DefIdMap};
use rustc_span::symbol::Ident;
Expand Down Expand Up @@ -50,19 +48,19 @@ impl Graph {

/// Children of a given impl, grouped into blanket/non-blanket varieties as is
/// done in `TraitDef`.
#[derive(Default, TyEncodable, TyDecodable, Debug)]
#[derive(Default, TyEncodable, TyDecodable, Debug, HashStable)]
pub struct Children {
// Impls of a trait (or specializations of a given impl). To allow for
// quicker lookup, the impls are indexed by a simplified version of their
// `Self` type: impls with a simplifiable `Self` are stored in
// `nonblanket_impls` keyed by it, while all other impls are stored in
// `non_blanket_impls` keyed by it, while all other impls are stored in
// `blanket_impls`.
//
// A similar division is used within `TraitDef`, but the lists there collect
// together *all* the impls for a trait, and are populated prior to building
// the specialization graph.
/// Impls of the trait.
pub nonblanket_impls: FxHashMap<SimplifiedType, Vec<DefId>>,
pub non_blanket_impls: FxIndexMap<SimplifiedType, Vec<DefId>>,

/// Blanket impls associated with the trait.
pub blanket_impls: Vec<DefId>,
Expand Down Expand Up @@ -235,11 +233,3 @@ pub fn ancestors(
})
}
}

impl<'a> HashStable<StableHashingContext<'a>> for Children {
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
let Children { ref nonblanket_impls, ref blanket_impls } = *self;

ich::hash_stable_trait_impls(hcx, hasher, blanket_impls, nonblanket_impls);
}
}
16 changes: 3 additions & 13 deletions compiler/rustc_middle/src/ty/trait_def.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::ich::{self, StableHashingContext};
use crate::traits::specialization_graph;
use crate::ty::fast_reject;
use crate::ty::fold::TypeFoldable;
Expand All @@ -7,8 +6,7 @@ use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::definitions::DefPathHash;

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::fx::FxIndexMap;
use rustc_errors::ErrorReported;
use rustc_macros::HashStable;

Expand Down Expand Up @@ -66,11 +64,11 @@ pub enum TraitSpecializationKind {
AlwaysApplicable,
}

#[derive(Default, Debug)]
#[derive(Default, Debug, HashStable)]
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>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅 ❤️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

impl TraitImpls {
Expand Down Expand Up @@ -249,11 +247,3 @@ pub(super) fn trait_impls_of_provider(tcx: TyCtxt<'_>, trait_id: DefId) -> Trait

impls
}

impl<'a> HashStable<StableHashingContext<'a>> for TraitImpls {
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
let TraitImpls { ref blanket_impls, ref non_blanket_impls } = *self;

ich::hash_stable_trait_impls(hcx, hasher, blanket_impls, non_blanket_impls);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl ChildrenExt for Children {
let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap();
if let Some(st) = fast_reject::simplify_type(tcx, trait_ref.self_ty(), false) {
debug!("insert_blindly: impl_def_id={:?} st={:?}", impl_def_id, st);
self.nonblanket_impls.entry(st).or_default().push(impl_def_id)
self.non_blanket_impls.entry(st).or_default().push(impl_def_id)
} else {
debug!("insert_blindly: impl_def_id={:?} st=None", impl_def_id);
self.blanket_impls.push(impl_def_id)
Expand All @@ -65,7 +65,7 @@ impl ChildrenExt for Children {
let vec: &mut Vec<DefId>;
if let Some(st) = fast_reject::simplify_type(tcx, trait_ref.self_ty(), false) {
debug!("remove_existing: impl_def_id={:?} st={:?}", impl_def_id, st);
vec = self.nonblanket_impls.get_mut(&st).unwrap();
vec = self.non_blanket_impls.get_mut(&st).unwrap();
} else {
debug!("remove_existing: impl_def_id={:?} st=None", impl_def_id);
vec = &mut self.blanket_impls;
Expand Down Expand Up @@ -216,15 +216,15 @@ impl ChildrenExt for Children {
}

fn iter_children(children: &mut Children) -> impl Iterator<Item = DefId> + '_ {
let nonblanket = children.nonblanket_impls.iter_mut().flat_map(|(_, v)| v.iter());
let nonblanket = children.non_blanket_impls.iter().flat_map(|(_, v)| v.iter());
children.blanket_impls.iter().chain(nonblanket).cloned()
}

fn filtered_children(
children: &mut Children,
st: SimplifiedType,
) -> impl Iterator<Item = DefId> + '_ {
let nonblanket = children.nonblanket_impls.entry(st).or_default().iter();
let nonblanket = children.non_blanket_impls.entry(st).or_default().iter();
children.blanket_impls.iter().chain(nonblanket).cloned()
}

Expand Down