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.

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

Jump to bottom

Collector tweaks #67756

Merged
merged 4 commits into from
Jan 11, 2020
Merged

Collector tweaks #67756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3ce144
Run codegen unit partitioning and assert_symbols_are_distinct in para…
Zoxc Dec 31, 2019
4beeadd
Fix a deadlock
Zoxc Dec 31, 2019
51a73eb
Avoid a duplicate hash map lookup
Zoxc Dec 31, 2019
4a64716
Add a comment
Zoxc Jan 10, 2020
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
28 changes: 14 additions & 14 deletions src/librustc_mir/monomorphize/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ use rustc_hir as hir;
use rustc_hir::def_id::{DefId, DefIdMap, LOCAL_CRATE};
use rustc_hir::itemlikevisit::ItemLikeVisitor;
use rustc_index::bit_set::GrowableBitSet;

use smallvec::SmallVec;
use std::iter;

#[derive(PartialEq)]
Expand Down Expand Up @@ -227,28 +227,23 @@ impl<'tcx> InliningMap<'tcx> {
}
}

fn record_accesses<I>(&mut self, source: MonoItem<'tcx>, new_targets: I)
where
I: Iterator<Item = (MonoItem<'tcx>, bool)> + ExactSizeIterator,
{
assert!(!self.index.contains_key(&source));

fn record_accesses(&mut self, source: MonoItem<'tcx>, new_targets: &[(MonoItem<'tcx>, bool)]) {
let start_index = self.targets.len();
let new_items_count = new_targets.len();
let new_items_count_total = new_items_count + self.targets.len();

self.targets.reserve(new_items_count);
self.inlines.ensure(new_items_count_total);

for (i, (target, inline)) in new_targets.enumerate() {
self.targets.push(target);
if inline {
for (i, (target, inline)) in new_targets.iter().enumerate() {
self.targets.push(*target);
if *inline {
self.inlines.insert(i + start_index);
}
}

let end_index = self.targets.len();
self.index.insert(source, (start_index, end_index));
assert!(self.index.insert(source, (start_index, end_index)).is_none());
}

// Internally iterate over all items referenced by `source` which will be
Expand Down Expand Up @@ -403,10 +398,15 @@ fn record_accesses<'tcx>(
mono_item.instantiation_mode(tcx) == InstantiationMode::LocalCopy
};

let accesses =
callees.into_iter().map(|mono_item| (*mono_item, is_inlining_candidate(mono_item)));
// We collect this into a `SmallVec` to avoid calling `is_inlining_candidate` in the lock.
// FIXME: Call `is_inlining_candidate` when pushing to `neighbors` in `collect_items_rec`
// instead to avoid creating this `SmallVec`.
let accesses: SmallVec<[_; 128]> = callees
Copy link
Member

Choose a reason for hiding this comment

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

We should add a comment here about the deadlock between is_inlining_candidate and inlining_map, if I'm following correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, a deadlock definitely seems noteworthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to note here other than don't call random code in locks =P

Copy link
Member

Choose a reason for hiding this comment

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

It is non-obvious to me at least when glancing at the code that there is a lock involved (and that is_inlining_candidate also takes it, if I'm recalling correctly). Explicitly noting that would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything pretty much uses a lock unless you know otherwise. We can't have explicit notes on all function calls.

Copy link
Member

Choose a reason for hiding this comment

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

My main concern is that someone will drop the SmallVec here as an "optimization". I also feel like the argument about all functions potentially locking is a bit odd - if it were really true, then we'd be unable to review code well. I might even argue that a refactor which explicitly passes the lock in would be helpful here, if possible; it's hopefully rare that we have such unexpected locking.

.into_iter()
.map(|mono_item| (*mono_item, is_inlining_candidate(mono_item)))
.collect();

inlining_map.lock_mut().record_accesses(caller, accesses);
inlining_map.lock_mut().record_accesses(caller, &accesses);
}

fn check_recursion_limit<'tcx>(
Expand Down
32 changes: 20 additions & 12 deletions src/librustc_mir/monomorphize/partitioning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ use rustc::ty::print::characteristic_def_id_of_type;
use rustc::ty::query::Providers;
use rustc::ty::{self, DefIdTree, InstanceDef, TyCtxt};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::sync;
use rustc_hir::def::DefKind;
use rustc_hir::def_id::{CrateNum, DefId, DefIdSet, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_span::symbol::Symbol;
Expand Down Expand Up @@ -796,6 +797,8 @@ where
I: Iterator<Item = &'a MonoItem<'tcx>>,
'tcx: 'a,
{
let _prof_timer = tcx.prof.generic_activity("assert_symbols_are_distinct");

let mut symbols: Vec<_> =
mono_items.map(|mono_item| (mono_item, mono_item.symbol_name(tcx))).collect();

Expand Down Expand Up @@ -869,18 +872,23 @@ fn collect_and_partition_mono_items(

tcx.sess.abort_if_errors();

assert_symbols_are_distinct(tcx, items.iter());

let strategy = if tcx.sess.opts.incremental.is_some() {
PartitioningStrategy::PerModule
} else {
PartitioningStrategy::FixedUnitCount(tcx.sess.codegen_units())
};

let codegen_units = partition(tcx, items.iter().cloned(), strategy, &inlining_map)
.into_iter()
.map(Arc::new)
.collect::<Vec<_>>();
let (codegen_units, _) = tcx.sess.time("partition_and_assert_distinct_symbols", || {
sync::join(
|| {
let strategy = if tcx.sess.opts.incremental.is_some() {
PartitioningStrategy::PerModule
} else {
PartitioningStrategy::FixedUnitCount(tcx.sess.codegen_units())
};

partition(tcx, items.iter().cloned(), strategy, &inlining_map)
.into_iter()
.map(Arc::new)
.collect::<Vec<_>>()
},
|| assert_symbols_are_distinct(tcx, items.iter()),
)
});

let mono_items: DefIdSet = items
.iter()
Expand Down