Skip to content

[WIP] Benchmarking for the parallel import resolution PR #158845 #159387

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.

Sign up for GitHub

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

Closed
petrochenkov wants to merge 1 commit into from
Closed

[WIP] Benchmarking for the parallel import resolution PR #158845 #159387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions compiler/rustc_data_structures/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
//! where noted otherwise, the type in column one is defined as a
//! newtype around the type from column two or three.
//!
//! | Type | Serial version | Parallel version |
//! | ----------------------- | ------------------- | ------------------------------- |
//! | `Lock<T>` | `RefCell<T>` | `RefCell<T>` or |
//! | | | `parking_lot::Mutex<T>` |
//! | `RwLock<T>` | `RefCell<T>` | `parking_lot::RwLock<T>` |
//! | Type | Serial version | Parallel version |
//! | ----------------------- | ------------------------ | ------------------------------- |
//! | `Lock<T>` | `RefCell<T>` | `RefCell<T>` or |
//! | | | `parking_lot::Mutex<T>` |
//! | `RwLock<T>` | `parking_lot::RwLock<T>` | `parking_lot::RwLock<T>` |

use std::collections::HashMap;
use std::hash::{BuildHasher, Hash};
Expand All @@ -38,8 +38,8 @@ pub use self::mode::{
FromDyn, check_dyn_thread_safe, is_dyn_thread_safe, set_dyn_thread_safe_mode,
};
pub use self::parallel::{
broadcast, par_fns, par_for_each_in, par_join, par_map, parallel_guard, spawn,
try_par_for_each_in,
broadcast, par_fns, par_for_each_in, par_for_each_slice, par_join, par_map, parallel_guard,
spawn, try_par_for_each_in,
};
pub use self::vec::{AppendOnlyIndexVec, AppendOnlyVec};
pub use self::worker_local::{Registry, WorkerLocal};
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_data_structures/src/sync/parallel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,14 @@ pub fn par_for_each_in<I: DynSend, T: IntoIterator<Item = I>>(
});
}

pub fn par_for_each_slice<T>(items: &mut [T], for_each: impl Fn(&mut T)) {
parallel_guard(|guard| {
items.iter_mut().for_each(|i| {
guard.run(|| for_each(i));
});
});
}

/// This runs `for_each` in parallel for each iterator item. If one or more of the
/// `for_each` calls returns `Err`, the function will also return `Err`. The error returned
/// will be non-deterministic, but this is expected to be used with `ErrorGuaranteed` which
Expand Down
77 changes: 50 additions & 27 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//! unexpanded macros in the fragment are visited and registered.
//! Imports are also considered items and placed into modules here, but not resolved yet.

use std::cell::RefMut;
use std::sync::Arc;

use rustc_ast::visit::{self, AssocCtxt, Visitor, WalkItemKind};
Expand Down Expand Up @@ -116,35 +117,57 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
if let module @ Some(..) = self.extern_module_map.borrow().get(&def_id) {
return module.map(|m| m.to_module());
}
// We need the lock on the extern_module_map for the entire duration of this call.
// It is otherwise entirely possible 2 different threads will create and allocate
// the exact same module during speculative resolution.
// FIXME(parallel_import_resolution): We lock the entire map to make sure
// no 2+ threads try to create the exact same module. Could it be possible to
// only "lock on" `def_id`?
let mut lock = self.extern_module_map.borrow_mut();
// No reentrant locking possible, so do a recursive call with lock
// passed as argument.
self.get_extern_module_with_lock(def_id, &mut lock)
}
}
}

// Query `def_kind` is not used because query system overhead is too expensive here.
let def_kind = self.cstore().def_kind_untracked(def_id);
if def_kind.is_module_like() {
let parent = self.tcx.opt_parent(def_id).map(|parent_id| {
self.get_nearest_non_block_module(parent_id).expect_extern()
});
// Query `expn_that_defined` is not used because
// hashing spans in its result is expensive.
let expn_id = self.cstore().expn_that_defined_untracked(self.tcx, def_id);
let module = self.new_extern_module(
parent,
ModuleKind::Def(
def_kind,
def_id,
DUMMY_NODE_ID,
Some(self.tcx.item_name(def_id)),
),
expn_id,
self.def_span(def_id),
// FIXME: Account for `#[no_implicit_prelude]` attributes.
parent.is_some_and(|module| module.no_implicit_prelude),
);
return Some(module.to_module());
fn get_extern_module_with_lock(
&self,
def_id: DefId,
map_lock: &mut RefMut<'_, FxIndexMap<DefId, ExternModule<'ra>>>,
) -> Option<Module<'ra>> {
if let module @ Some(..) = map_lock.get(&def_id) {
return module.map(|m| m.to_module());
}
// Query `def_kind` is not used because query system overhead is too expensive here.
let def_kind = self.cstore().def_kind_untracked(def_id);
if def_kind.is_module_like() {
let parent = self.tcx.opt_parent(def_id).map(|mut parent_id| {
loop {
match self.get_extern_module_with_lock(parent_id, map_lock) {
Some(module) => break module.expect_extern(),
None => parent_id = self.tcx.parent(parent_id),
}
}

None
}
});
// Query `expn_that_defined` is not used because
// hashing spans in its result is expensive.
let expn_id = self.cstore().expn_that_defined_untracked(self.tcx, def_id);
let module = ExternModule::new(
parent,
ModuleKind::Def(def_kind, def_id, DUMMY_NODE_ID, Some(self.tcx.item_name(def_id))),
self.tcx.visibility(def_id),
expn_id,
self.def_span(def_id),
// FIXME: Account for `#[no_implicit_prelude]` attributes.
parent.is_some_and(|module| module.no_implicit_prelude),
self.arenas,
);
map_lock.insert(def_id, module);
return Some(module.to_module());
}

None
}

pub(crate) fn expn_def_scope(&self, expn_id: ExpnId) -> Module<'ra> {
Expand Down Expand Up @@ -537,7 +560,7 @@ impl<'a, 'ra, 'tcx> DefCollector<'a, 'ra, 'tcx> {
on_unknown_attr: OnUnknownData::from_attrs(self.r, &item.attrs),
});

self.r.indeterminate_imports.push(import);
self.r.indeterminate_imports.push((import, None, 0));
match import.kind {
ImportKind::Single { target, .. } => {
// Don't add underscore imports to `single_imports`
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_resolve/src/ident.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
// used to avoid long scope chains, see the comments on `MacroRulesScopeRef`.
// As another consequence of this optimization visitors never observe invocation
// scopes for macros that were already expanded.
while let MacroRulesScope::Invocation(invoc_id) = macro_rules_scope.get() {
if let Some(next_scope) = self.output_macro_rules_scopes.get(&invoc_id) {
macro_rules_scope.set(next_scope.get());
let mut scope = macro_rules_scope.get();
while let MacroRulesScope::Invocation(invoc_id) = scope {
if let Some(next) = self.output_macro_rules_scopes.get(&invoc_id) {
scope = next.get();
macro_rules_scope.set(scope);
} else {
break;
}
Expand Down
82 changes: 49 additions & 33 deletions compiler/rustc_resolve/src/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ pub(crate) enum PendingDecl<'ra> {
}

enum ImportResolutionKind<'ra> {
// these are the decls the import imports, not the import declarations themselves
Single(PerNS<PendingDecl<'ra>>),
Glob(Vec<(Decl<'ra>, BindingKey, Span /* orig_ident_span */)>),
}

struct ImportResolution<'ra> {
pub(crate) struct ImportResolution<'ra> {
kind: ImportResolutionKind<'ra>,
imported_module: ModuleOrUniformRoot<'ra>,
}
Expand Down Expand Up @@ -312,9 +313,10 @@ impl<'ra> NameResolution<'ra> {

// module to keep the TLS private and only accessible through the function `enter_cycle_detector`.
pub(crate) mod cycle_detection {
use std::cell::RefCell;
use std::ptr;

use crate::{BindingKey, CacheRefCell, LocalModule};
use crate::{BindingKey, LocalModule};

thread_local!(
/// During import resolution, recursive imports can form cycles.
Expand All @@ -326,7 +328,7 @@ pub(crate) mod cycle_detection {
/// in the `Resolver Arenas` (lifetime `'ra`), it is thus stable and allows casting
/// to a `*const ()` for comparison. This is done because we can't use lifetimes
/// other than `'static` in thread local storage.
static ACTIVE_RESOLUTIONS: CacheRefCell<Vec<(*const (), BindingKey)>> = Default::default();
static ACTIVE_RESOLUTIONS: RefCell<Vec<(*const (), BindingKey)>> = Default::default();
);

pub(crate) struct ActiveResolutionGuard {
Expand Down Expand Up @@ -776,30 +778,43 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
while indeterminate_count < prev_indeterminate_count {
prev_indeterminate_count = indeterminate_count;
indeterminate_count = 0;
let mut resolutions = Vec::new();

self.assert_speculative = true;
for import in mem::take(&mut self.indeterminate_imports) {
let (resolution, import_indeterminate_count) = self.cm().resolve_import(import);
indeterminate_count += import_indeterminate_count;
match import_indeterminate_count {
0 => self.determined_imports.push(import),
_ => self.indeterminate_imports.push(import),
}
if let Some(resolution) = resolution {
resolutions.push((import, resolution));
}
}

let mut to_resolve_imports = mem::take(&mut self.indeterminate_imports);
let cm_resolver = self.cm();

rustc_data_structures::sync::par_for_each_slice(
&mut to_resolve_imports,
|(import, resolution, indeterminate_count)| {
let (new_resolution, import_indeterminate_count) =
cm_resolver.reborrow_ref().resolve_import(*import);
*resolution = new_resolution;
*indeterminate_count = import_indeterminate_count
},
);
self.assert_speculative = false;
self.write_import_resolutions(resolutions);

self.write_import_resolutions(&to_resolve_imports);

self.indeterminate_imports = to_resolve_imports
.extract_if(.., |(_, _, count)| {
indeterminate_count += *count;
*count > 0
})
.collect();
self.determined_imports.extend(to_resolve_imports.into_iter().map(|(i, _, _)| i));
}
}

fn write_import_resolutions(
&mut self,
import_resolutions: Vec<(Import<'ra>, ImportResolution<'ra>)>,
import_resolutions: &[(Import<'ra>, Option<ImportResolution<'ra>>, usize)],
) {
for (import, resolution) in &import_resolutions {
let ImportResolution { imported_module, .. } = resolution;
for (import, resolution, _) in import_resolutions {
let Some(ImportResolution { imported_module, .. }) = resolution else {
continue;
};
import.imported_module.set(Some(*imported_module), self);

if import.is_glob()
Expand All @@ -811,8 +826,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
}
}

for (import, resolution) in import_resolutions {
let ImportResolution { imported_module, kind: resolution_kind } = resolution;
for (import, resolution, _) in import_resolutions {
let Some(ImportResolution { imported_module, kind: resolution_kind }) = resolution
else {
continue;
};

match (&import.kind, resolution_kind) {
(
Expand All @@ -823,7 +841,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
match import_decls[ns] {
PendingDecl::Ready(Some(decl)) => {
// We need the `target`, `source` can be extracted.
let import_decl = this.new_import_decl(decl, import);
let import_decl = this.new_import_decl(decl, *import);
if import_decl.is_assoc_item()
&& !this.features.import_trait_associated_functions()
{
Expand Down Expand Up @@ -852,7 +870,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
key,
target.span,
|_, resolution| {
resolution.single_imports.swap_remove(&import);
resolution.single_imports.swap_remove(import);
},
);
}
Expand All @@ -879,11 +897,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
}

for (binding, key, orig_ident_span) in imported_decls {
let import_decl = self.new_import_decl(binding, import);
let import_decl = self.new_import_decl(*binding, *import);
let _ = self
.try_plant_decl_into_local_module(
key.ident,
orig_ident_span,
*orig_ident_span,
key.ns,
import_decl,
)
Expand Down Expand Up @@ -918,7 +936,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
for (is_indeterminate, import) in determined_imports
.iter()
.map(|i| (false, i))
.chain(indeterminate_imports.iter().map(|i| (true, i)))
.chain(indeterminate_imports.iter().map(|(i, _, _)| (true, i)))
{
let unresolved_import_error = self.finalize_import(*import);
// If this import is unresolved then create a dummy import
Expand Down Expand Up @@ -959,7 +977,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
return;
}

for import in &indeterminate_imports {
for (import, _, _) in &indeterminate_imports {
let path = import_path_to_string(
&import.module_path.iter().map(|seg| seg.ident).collect::<Vec<_>>(),
&import.kind,
Expand Down Expand Up @@ -1139,7 +1157,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
_ => unreachable!(),
};

let mut import_decls = PerNS::default();
let mut decls = PerNS::default();
let mut indeterminate_count = 0;
self.per_ns_cm(|mut this, ns| {
if bindings[ns].get() != PendingDecl::Pending {
Expand All @@ -1160,12 +1178,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
PendingDecl::Pending
}
};
import_decls[ns] = pending_decl;
decls[ns] = pending_decl;
});
let import_resolution = ImportResolution {
imported_module: module,
kind: ImportResolutionKind::Single(import_decls),
};
let import_resolution =
ImportResolution { imported_module: module, kind: ImportResolutionKind::Single(decls) };

(Some(import_resolution), indeterminate_count)
}
Expand Down
Loading
Loading