Skip to content

Tweak query code for performance #56613

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

Merged
merged 6 commits into from
Dec 19, 2018
Merged
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
23 changes: 13 additions & 10 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
@@ -162,7 +162,9 @@ macro_rules! define_dep_nodes {
}
}

#[inline]
// FIXME: Make `is_anon`, `is_input`, `is_eval_always` and `has_params` properties
// of queries
#[inline(always)]
pub fn is_anon(&self) -> bool {
match *self {
$(
@@ -171,7 +173,7 @@ macro_rules! define_dep_nodes {
}
}

#[inline]
#[inline(always)]
pub fn is_input(&self) -> bool {
match *self {
$(
@@ -180,7 +182,7 @@ macro_rules! define_dep_nodes {
}
}

#[inline]
#[inline(always)]
pub fn is_eval_always(&self) -> bool {
match *self {
$(
@@ -190,7 +192,7 @@ macro_rules! define_dep_nodes {
}

#[allow(unreachable_code)]
#[inline]
#[inline(always)]
pub fn has_params(&self) -> bool {
match *self {
$(
@@ -230,6 +232,7 @@ macro_rules! define_dep_nodes {

impl DepNode {
#[allow(unreachable_code, non_snake_case)]
#[inline(always)]
pub fn new<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
dep: DepConstructor<'gcx>)
-> DepNode
@@ -299,11 +302,11 @@ macro_rules! define_dep_nodes {
/// Construct a DepNode from the given DepKind and DefPathHash. This
/// method will assert that the given DepKind actually requires a
/// single DefId/DefPathHash parameter.
#[inline]
#[inline(always)]
pub fn from_def_path_hash(kind: DepKind,
def_path_hash: DefPathHash)
-> DepNode {
Copy link
Member

Choose a reason for hiding this comment

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

You can turn the assertion below into a debug_assertion.

assert!(kind.can_reconstruct_query_key() && kind.has_params());
debug_assert!(kind.can_reconstruct_query_key() && kind.has_params());
DepNode {
kind,
hash: def_path_hash.0,
@@ -313,9 +316,9 @@ macro_rules! define_dep_nodes {
/// Create a new, parameterless DepNode. This method will assert
/// that the DepNode corresponding to the given DepKind actually
/// does not require any parameters.
#[inline]
#[inline(always)]
pub fn new_no_params(kind: DepKind) -> DepNode {
assert!(!kind.has_params());
debug_assert!(!kind.has_params());
DepNode {
kind,
hash: Fingerprint::ZERO,
@@ -418,14 +421,14 @@ impl fmt::Debug for DepNode {


impl DefPathHash {
#[inline]
#[inline(always)]
pub fn to_dep_node(self, kind: DepKind) -> DepNode {
DepNode::from_def_path_hash(kind, self)
}
}

impl DefId {
#[inline]
#[inline(always)]
pub fn to_dep_node(self, tcx: TyCtxt<'_, '_, '_>, kind: DepKind) -> DepNode {
DepNode::from_def_path_hash(kind, tcx.def_path_hash(self))
}
7 changes: 7 additions & 0 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
@@ -159,6 +159,13 @@ impl Forest {
self.dep_graph.read(DepNode::new_no_params(DepKind::Krate));
&self.krate
}

/// This is internally in the depedency tracking system.
/// Use the `krate` method to ensure your dependency on the
/// crate is tracked.
pub fn untracked_krate<'hir>(&'hir self) -> &'hir Crate {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a warning comment here not to use this function and that it only exists for internal use with the dep-tracking system.

&self.krate
}
}

/// Represents a mapping from Node IDs to AST elements and their parent
1 change: 1 addition & 0 deletions src/librustc/ich/hcx.rs
Original file line number Diff line number Diff line change
@@ -86,6 +86,7 @@ impl<'a> StableHashingContext<'a> {
// The `krate` here is only used for mapping BodyIds to Bodies.
// Don't use it for anything else or you'll run the risk of
// leaking data out of the tracking system.
#[inline]
pub fn new(sess: &'a Session,
krate: &'a hir::Crate,
definitions: &'a Definitions,
2 changes: 2 additions & 0 deletions src/librustc/lib.rs
Original file line number Diff line number Diff line change
@@ -60,10 +60,12 @@
#![feature(slice_sort_by_cached_key)]
#![feature(specialization)]
#![feature(unboxed_closures)]
#![feature(thread_local)]
#![feature(trace_macros)]
#![feature(trusted_len)]
#![feature(vec_remove_item)]
#![feature(step_trait)]
#![feature(stmt_expr_attributes)]
#![feature(integer_atomics)]
#![feature(test)]
#![feature(in_band_lifetimes)]
20 changes: 17 additions & 3 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
@@ -131,6 +131,9 @@ pub struct Session {
/// Used by -Z profile-queries in util::common
pub profile_channel: Lock<Option<mpsc::Sender<ProfileQueriesMsg>>>,

/// Used by -Z self-profile
pub self_profiling_active: bool,

/// Used by -Z self-profile
pub self_profiling: Lock<SelfProfiler>,

@@ -823,10 +826,17 @@ impl Session {
}
}

#[inline(never)]
#[cold]
fn profiler_active<F: FnOnce(&mut SelfProfiler) -> ()>(&self, f: F) {
let mut profiler = self.self_profiling.borrow_mut();
f(&mut profiler);
}

#[inline(always)]
pub fn profiler<F: FnOnce(&mut SelfProfiler) -> ()>(&self, f: F) {
if self.opts.debugging_opts.self_profile || self.opts.debugging_opts.profile_json {
let mut profiler = self.self_profiling.borrow_mut();
f(&mut profiler);
if unlikely!(self.self_profiling_active) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LLVM already assigns reduced branch weights to branches postdominated by calls to cold functions, so I wouldn't expect this unlikely!() to have an effect 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.

I did find a case where it didn't though, so I used likely and unlikely just in case.

Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to have PGO for the Rust compiler at some point :)

self.profiler_active(f)
}
}

@@ -1145,6 +1155,9 @@ pub fn build_session_(
CguReuseTracker::new_disabled()
};

let self_profiling_active = sopts.debugging_opts.self_profile ||
sopts.debugging_opts.profile_json;

let sess = Session {
target: target_cfg,
host,
@@ -1177,6 +1190,7 @@ pub fn build_session_(
imported_macro_spans: OneThread::new(RefCell::new(FxHashMap::default())),
incr_comp_session: OneThread::new(RefCell::new(IncrCompSession::NotInitialized)),
cgu_reuse_tracker,
self_profiling_active,
self_profiling: Lock::new(SelfProfiler::new()),
profile_channel: Lock::new(None),
perf_stats: PerfStats {
3 changes: 2 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
@@ -1336,8 +1336,9 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
self.cstore.crate_data_as_rc_any(cnum)
}

#[inline(always)]
pub fn create_stable_hashing_context(self) -> StableHashingContext<'a> {
let krate = self.dep_graph.with_ignore(|| self.hir().krate());
let krate = self.gcx.hir_map.forest.untracked_krate();

StableHashingContext::new(self.sess,
krate,
69 changes: 41 additions & 28 deletions src/librustc/ty/query/job.rs
Original file line number Diff line number Diff line change
@@ -18,6 +18,11 @@ use syntax_pos::Span;
use ty::tls;
use ty::query::Query;
use ty::query::plumbing::CycleError;
#[cfg(not(parallel_queries))]
use ty::query::{
plumbing::TryGetJob,
config::QueryDescription,
};
use ty::context::TyCtxt;
use errors::Diagnostic;
use std::process;
@@ -83,44 +88,52 @@ impl<'tcx> QueryJob<'tcx> {
///
/// For single threaded rustc there's no concurrent jobs running, so if we are waiting for any
/// query that means that there is a query cycle, thus this always running a cycle error.
pub(super) fn await<'lcx>(
#[cfg(not(parallel_queries))]
#[inline(never)]
#[cold]
pub(super) fn cycle_error<'lcx, 'a, D: QueryDescription<'tcx>>(
&self,
tcx: TyCtxt<'_, 'tcx, 'lcx>,
span: Span,
) -> Result<(), CycleError<'tcx>> {
#[cfg(not(parallel_queries))]
{
self.find_cycle_in_stack(tcx, span)
}
) -> TryGetJob<'a, 'tcx, D> {
TryGetJob::JobCompleted(Err(Box::new(self.find_cycle_in_stack(tcx, span))))
}

#[cfg(parallel_queries)]
{
tls::with_related_context(tcx, move |icx| {
let mut waiter = Lrc::new(QueryWaiter {
query: icx.query.clone(),
span,
cycle: Lock::new(None),
condvar: Condvar::new(),
});
self.latch.await(&waiter);
// FIXME: Get rid of this lock. We have ownership of the QueryWaiter
// although another thread may still have a Lrc reference so we cannot
// use Lrc::get_mut
let mut cycle = waiter.cycle.lock();
match cycle.take() {
None => Ok(()),
Some(cycle) => Err(cycle)
}
})
}
/// Awaits for the query job to complete.
///
/// For single threaded rustc there's no concurrent jobs running, so if we are waiting for any
/// query that means that there is a query cycle, thus this always running a cycle error.
#[cfg(parallel_queries)]
pub(super) fn await<'lcx>(
&self,
tcx: TyCtxt<'_, 'tcx, 'lcx>,
span: Span,
) -> Result<(), Box<CycleError<'tcx>>> {
tls::with_related_context(tcx, move |icx| {
let mut waiter = Lrc::new(QueryWaiter {
query: icx.query.clone(),
span,
cycle: Lock::new(None),
condvar: Condvar::new(),
});
self.latch.await(&waiter);
// FIXME: Get rid of this lock. We have ownership of the QueryWaiter
// although another thread may still have a Lrc reference so we cannot
// use Lrc::get_mut
let mut cycle = waiter.cycle.lock();
match cycle.take() {
None => Ok(()),
Some(cycle) => Err(Box::new(cycle))
}
})
}

#[cfg(not(parallel_queries))]
fn find_cycle_in_stack<'lcx>(
&self,
tcx: TyCtxt<'_, 'tcx, 'lcx>,
span: Span,
) -> Result<(), CycleError<'tcx>> {
) -> CycleError<'tcx> {
// Get the current executing query (waiter) and find the waitee amongst its parents
let mut current_job = tls::with_related_context(tcx, |icx| icx.query.clone());
let mut cycle = Vec::new();
@@ -140,7 +153,7 @@ impl<'tcx> QueryJob<'tcx> {
let usage = job.parent.as_ref().map(|parent| {
(job.info.span, parent.info.query.clone())
});
return Err(CycleError { usage, cycle });
return CycleError { usage, cycle };
}

current_job = job.parent.clone();
6 changes: 3 additions & 3 deletions src/librustc/ty/query/mod.rs
Original file line number Diff line number Diff line change
@@ -705,21 +705,21 @@ impl<'a, 'tcx, 'lcx> TyCtxt<'a, 'tcx, 'lcx> {
self,
span: Span,
key: DefId,
) -> Result<&'tcx [Ty<'tcx>], DiagnosticBuilder<'a>> {
) -> Result<&'tcx [Ty<'tcx>], Box<DiagnosticBuilder<'a>>> {
self.try_get_query::<queries::adt_sized_constraint<'_>>(span, key)
}
pub fn try_needs_drop_raw(
self,
span: Span,
key: ty::ParamEnvAnd<'tcx, Ty<'tcx>>,
) -> Result<bool, DiagnosticBuilder<'a>> {
) -> Result<bool, Box<DiagnosticBuilder<'a>>> {
self.try_get_query::<queries::needs_drop_raw<'_>>(span, key)
}
pub fn try_optimized_mir(
self,
span: Span,
key: DefId,
) -> Result<&'tcx mir::Mir<'tcx>, DiagnosticBuilder<'a>> {
) -> Result<&'tcx mir::Mir<'tcx>, Box<DiagnosticBuilder<'a>>> {
self.try_get_query::<queries::optimized_mir<'_>>(span, key)
}
}
87 changes: 57 additions & 30 deletions src/librustc/ty/query/plumbing.rs
Original file line number Diff line number Diff line change
@@ -153,8 +153,18 @@ impl<'a, 'tcx, Q: QueryDescription<'tcx>> JobOwner<'a, 'tcx, Q> {
};
mem::drop(lock);

if let Err(cycle) = job.await(tcx, span) {
return TryGetJob::JobCompleted(Err(cycle));
// If we are single-threaded we know that we have cycle error,
// so we just turn the errror
#[cfg(not(parallel_queries))]
return job.cycle_error(tcx, span);

// With parallel queries we might just have to wait on some other
// thread
#[cfg(parallel_queries)]
{
if let Err(cycle) = job.await(tcx, span) {
return TryGetJob::JobCompleted(Err(cycle));
}
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 the following would be more readable:

if cfg!(parallel_queries) {
    if let Err(cycle) = job.await(tcx, span) {
        return TryGetJob::JobCompleted(Err(cycle));
    }
} else {
    // If we get here in non-parallel mode we know that we have cycle error.
    let result = job.await(tcx, span);
    debug_assert!(result.is_err());
    return result;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't work since await now has different signatures depending on cfg!(parallel_queries). I'll add the comment though.

Copy link
Member

Choose a reason for hiding this comment

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

👍

}
}
}
@@ -241,12 +251,16 @@ pub(super) enum TryGetJob<'a, 'tcx: 'a, D: QueryDescription<'tcx> + 'a> {
/// The query was already completed.
/// Returns the result of the query and its dep node index
/// if it succeeded or a cycle error if it failed
JobCompleted(Result<(D::Value, DepNodeIndex), CycleError<'tcx>>),
JobCompleted(Result<(D::Value, DepNodeIndex), Box<CycleError<'tcx>>>),
}

impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub(super) fn report_cycle(self, CycleError { usage, cycle: stack }: CycleError<'gcx>)
-> DiagnosticBuilder<'a>
#[inline(never)]
#[cold]
pub(super) fn report_cycle(
self,
box CycleError { usage, cycle: stack }: Box<CycleError<'gcx>>
) -> Box<DiagnosticBuilder<'a>>
{
assert!(!stack.is_empty());

@@ -280,7 +294,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
&format!("cycle used when {}", query.describe(self)));
}

return err
return Box::new(err)
})
}

@@ -345,11 +359,12 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

#[inline(never)]
fn try_get_with<Q: QueryDescription<'gcx>>(
self,
span: Span,
key: Q::Key)
-> Result<Q::Value, CycleError<'gcx>>
-> Result<Q::Value, Box<CycleError<'gcx>>>
{
debug!("ty::queries::{}::try_get_with(key={:?}, span={:?})",
Q::NAME,
@@ -436,7 +451,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
job: JobOwner<'a, 'gcx, Q>,
dep_node_index: DepNodeIndex,
dep_node: &DepNode
) -> Result<Q::Value, CycleError<'gcx>>
) -> Result<Q::Value, Box<CycleError<'gcx>>>
{
// Note this function can be called concurrently from the same query
// We must ensure that this is handled correctly
@@ -522,7 +537,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
key: Q::Key,
job: JobOwner<'_, 'gcx, Q>,
dep_node: DepNode)
-> Result<(Q::Value, DepNodeIndex), CycleError<'gcx>> {
-> Result<(Q::Value, DepNodeIndex), Box<CycleError<'gcx>>> {
// If the following assertion triggers, it can have two reasons:
// 1. Something is wrong with DepNode creation, either here or
// in DepGraph::try_mark_green()
@@ -611,37 +626,55 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
key: Q::Key,
span: Span,
dep_node: DepNode
) -> Result<(Q::Value, DepNodeIndex), CycleError<'gcx>> {
) {
profq_msg!(
self,
ProfileQueriesMsg::QueryBegin(span.data(), profq_query_msg!(Q::NAME, self, key))
);

// We may be concurrently trying both execute and force a query
// Ensure that only one of them runs the query
let job = match JobOwner::try_get(self, span, &key) {
TryGetJob::NotYetStarted(job) => job,
TryGetJob::JobCompleted(result) => return result,
TryGetJob::JobCompleted(_) => return,
};
self.force_query_with_job::<Q>(key, job, dep_node)
if let Err(e) = self.force_query_with_job::<Q>(key, job, dep_node) {
self.report_cycle(e).emit();
}
}

pub(super) fn try_get_query<Q: QueryDescription<'gcx>>(
self,
span: Span,
key: Q::Key,
) -> Result<Q::Value, DiagnosticBuilder<'a>> {
) -> Result<Q::Value, Box<DiagnosticBuilder<'a>>> {
match self.try_get_with::<Q>(span, key) {
Ok(e) => Ok(e),
Err(e) => Err(self.report_cycle(e)),
}
}

// FIXME: Try uninlining this
#[inline(always)]
pub(super) fn get_query<Q: QueryDescription<'gcx>>(
self,
span: Span,
key: Q::Key,
) -> Q::Value {
self.try_get_query::<Q>(span, key).unwrap_or_else(|mut e| {
e.emit();
Q::handle_cycle_error(self)
self.try_get_with::<Q>(span, key).unwrap_or_else(|e| {
self.emit_error::<Q>(e)
})
}

#[inline(never)]
#[cold]
fn emit_error<Q: QueryDescription<'gcx>>(
self,
e: Box<CycleError<'gcx>>,
) -> Q::Value {
self.report_cycle(e).emit();
Q::handle_cycle_error(self)
}
}

macro_rules! handle_cycle_error {
@@ -806,15 +839,18 @@ macro_rules! define_queries_inner {
}

impl<$tcx> QueryAccessors<$tcx> for queries::$name<$tcx> {
#[inline(always)]
fn query(key: Self::Key) -> Query<'tcx> {
Query::$name(key)
}

#[inline(always)]
fn query_cache<'a>(tcx: TyCtxt<'a, $tcx, '_>) -> &'a Lock<QueryCache<$tcx, Self>> {
&tcx.queries.$name
}

#[allow(unused)]
#[inline(always)]
fn to_dep_node(tcx: TyCtxt<'_, $tcx, '_>, key: &Self::Key) -> DepNode {
use dep_graph::DepConstructor::*;

@@ -861,6 +897,7 @@ macro_rules! define_queries_inner {

impl<'a, 'gcx, 'tcx> Deref for TyCtxtAt<'a, 'gcx, 'tcx> {
type Target = TyCtxt<'a, 'gcx, 'tcx>;
#[inline(always)]
fn deref(&self) -> &Self::Target {
&self.tcx
}
@@ -869,6 +906,7 @@ macro_rules! define_queries_inner {
impl<'a, $tcx, 'lcx> TyCtxt<'a, $tcx, 'lcx> {
/// Return a transparent wrapper for `TyCtxt` which uses
/// `span` as the location of queries performed through it.
#[inline(always)]
pub fn at(self, span: Span) -> TyCtxtAt<'a, $tcx, 'lcx> {
TyCtxtAt {
tcx: self,
@@ -877,13 +915,15 @@ macro_rules! define_queries_inner {
}

$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: $K) -> $V {
self.at(DUMMY_SP).$name(key)
})*
}

impl<'a, $tcx, 'lcx> TyCtxtAt<'a, $tcx, 'lcx> {
$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: $K) -> $V {
self.tcx.get_query::<queries::$name<'_>>(self.span, key)
})*
@@ -1023,20 +1063,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
macro_rules! force {
($query:ident, $key:expr) => {
{
use $crate::util::common::{ProfileQueriesMsg, profq_msg};

profq_msg!(tcx,
ProfileQueriesMsg::QueryBegin(
DUMMY_SP.data(),
profq_query_msg!(::ty::query::queries::$query::NAME, tcx, $key),
)
);

if let Err(e) = tcx.force_query::<::ty::query::queries::$query<'_>>(
$key, DUMMY_SP, *dep_node
) {
tcx.report_cycle(e).emit();
}
tcx.force_query::<::ty::query::queries::$query<'_>>($key, DUMMY_SP, *dep_node);
}
}
};
1 change: 1 addition & 0 deletions src/librustc_data_structures/fingerprint.rs
Original file line number Diff line number Diff line change
@@ -86,6 +86,7 @@ impl ::std::fmt::Display for Fingerprint {
}

impl stable_hasher::StableHasherResult for Fingerprint {
#[inline]
fn finish(hasher: stable_hasher::StableHasher<Self>) -> Self {
let (_0, _1) = hasher.finalize();
Fingerprint(_0, _1)
22 changes: 22 additions & 0 deletions src/librustc_data_structures/lib.rs
Original file line number Diff line number Diff line change
@@ -30,6 +30,8 @@
#![feature(allow_internal_unstable)]
#![feature(vec_resize_with)]
#![feature(hash_raw_entry)]
#![feature(stmt_expr_attributes)]
#![feature(core_intrinsics)]

#![cfg_attr(unix, feature(libc))]
#![cfg_attr(test, feature(test))]
@@ -58,6 +60,26 @@ extern crate rustc_cratesio_shim;

pub use rustc_serialize::hex::ToHex;

#[macro_export]
macro_rules! likely {
Copy link
Member

Choose a reason for hiding this comment

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

This macro isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets used in #56614. Might as well add them both

Copy link
Member

Choose a reason for hiding this comment

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

Didn't look at that PR :)

($e:expr) => {
#[allow(unused_unsafe)]
{
unsafe { std::intrinsics::likely($e) }
}
}
}

#[macro_export]
macro_rules! unlikely {
($e:expr) => {
#[allow(unused_unsafe)]
{
unsafe { std::intrinsics::unlikely($e) }
}
}
}

pub mod macros;
pub mod svh;
pub mod base_n;
1 change: 1 addition & 0 deletions src/libsyntax/parse/mod.rs
Original file line number Diff line number Diff line change
@@ -81,6 +81,7 @@ impl ParseSess {
}
}

#[inline]
pub fn source_map(&self) -> &SourceMap {
&self.source_map
}