Skip to content

Split execute_job into execute_job_incr and execute_job_non_incr #109046

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

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Split execute_job into execute_job_incr and execute_job_non_incr #109046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
486a387
Split `execute_job` into `execute_job_incr` and `execute_job_non_incr`
Zoxc Mar 10, 2023
c4bcac6
Add some assertions
Zoxc Mar 19, 2023
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
1 change: 1 addition & 0 deletions compiler/rustc_query_system/src/dep_graph/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,7 @@ impl<K: DepKind> DepGraph<K> {
}

pub(crate) fn next_virtual_depnode_index(&self) -> DepNodeIndex {
debug_assert!(self.data.is_none());
let index = self.virtual_dep_node_index.fetch_add(1, Relaxed);
DepNodeIndex::from_u32(index)
}
Expand Down
71 changes: 41 additions & 30 deletions compiler/rustc_query_system/src/query/plumbing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,11 @@ where

match JobOwner::<'_, Q::Key, Qcx::DepKind>::try_start(&qcx, state, state_lock, span, key) {
TryGetJob::NotYetStarted(job) => {
let (result, dep_node_index) = execute_job(query, qcx, key.clone(), dep_node, job.id);
let (result, dep_node_index) = match qcx.dep_context().dep_graph().data() {
None => execute_job_non_incr(query, qcx, key, job.id),
Some(data) => execute_job_incr(query, qcx, data, key, dep_node, job.id),
};

let cache = query.query_cache(qcx);
if query.feedable() {
// We should not compute queries that also got a value via feeding.
Expand Down Expand Up @@ -413,48 +417,55 @@ where
}
}

// Fast path for when incr. comp. is off.
#[inline(always)]
fn execute_job<Q, Qcx>(
fn execute_job_non_incr<Q, Qcx>(
query: Q,
qcx: Qcx,
key: Q::Key,
mut dep_node_opt: Option<DepNode<Qcx::DepKind>>,
job_id: QueryJobId,
) -> (Q::Value, DepNodeIndex)
where
Q: QueryConfig<Qcx>,
Qcx: QueryContext,
{
let dep_graph = qcx.dep_context().dep_graph();
let dep_graph_data = match dep_graph.data() {
// Fast path for when incr. comp. is off.
None => {
// Fingerprint the key, just to assert that it doesn't
// have anything we don't consider hashable
if cfg!(debug_assertions) {
let _ = key.to_fingerprint(*qcx.dep_context());
}
debug_assert!(!qcx.dep_context().dep_graph().is_fully_enabled());

let prof_timer = qcx.dep_context().profiler().query_provider();
let result =
qcx.start_query(job_id, query.depth_limit(), None, || query.compute(qcx, key));
let dep_node_index = dep_graph.next_virtual_depnode_index();
prof_timer.finish_with_query_invocation_id(dep_node_index.into());

// Similarly, fingerprint the result to assert that
// it doesn't have anything not considered hashable.
if cfg!(debug_assertions) && let Some(hash_result) = query.hash_result()
{
qcx.dep_context().with_stable_hashing_context(|mut hcx| {
hash_result(&mut hcx, &result);
});
}
// Fingerprint the key, just to assert that it doesn't
// have anything we don't consider hashable
if cfg!(debug_assertions) {
let _ = key.to_fingerprint(*qcx.dep_context());
}

return (result, dep_node_index);
}
Some(data) => data,
};
let prof_timer = qcx.dep_context().profiler().query_provider();
let result = qcx.start_query(job_id, query.depth_limit(), None, || query.compute(qcx, key));
let dep_node_index = qcx.dep_context().dep_graph().next_virtual_depnode_index();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you encapsulate next_virtual_depnode_index? The goal is to make sure that it is only called when DepGraph::data is None.

For instance, making DepGraph::Data return some marker type DisabledDepGraph on which we can call that method?

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 tried to make a marker type struct DisabledDepGraph<'a, K: DepKind>(&'a DepGraph<K>), but it doesn't seem like LLVM was able to optimize it away:

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.7102s1.7175s 0.43%
🟣 hyper:check0.2533s0.2538s 0.23%
🟣 regex:check0.9534s0.9567s 0.35%
🟣 syn:check1.5869s1.5971s 0.64%
🟣 syntex_syntax:check6.1097s6.1317s 0.36%
Total10.6134s10.6568s 0.41%
Summary1.0000s1.0040s 0.40%

Copy link
Contributor

Choose a reason for hiding this comment

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

What about DisabledDepGraph(Lrc<AtomicU32>), and have the dep-graph hand out references to that struct?

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 sounds very performance equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

How about adding assert!(self.data.is_none()); to DepGraph::next_virtual_depnode_index()?

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 seems to be cheaper:

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.7151s1.7189s 0.22%
🟣 hyper:check0.2516s0.2525s 0.34%
🟣 regex:check0.9532s0.9538s 0.06%
🟣 syn:check1.5406s1.5400s -0.04%
🟣 syntex_syntax:check5.9116s5.9193s 0.13%
Total10.3722s10.3845s 0.12%
Summary1.0000s1.0014s 0.14%

There's a 0.01% code size increase so it probably doesn't get optimized away.

prof_timer.finish_with_query_invocation_id(dep_node_index.into());

// Similarly, fingerprint the result to assert that
// it doesn't have anything not considered hashable.
if cfg!(debug_assertions) && let Some(hash_result) = query.hash_result() {
qcx.dep_context().with_stable_hashing_context(|mut hcx| {
hash_result(&mut hcx, &result);
});
}

(result, dep_node_index)
}

#[inline(always)]
fn execute_job_incr<Q, Qcx>(
query: Q,
qcx: Qcx,
dep_graph_data: &DepGraphData<Qcx::DepKind>,
key: Q::Key,
mut dep_node_opt: Option<DepNode<Qcx::DepKind>>,
job_id: QueryJobId,
) -> (Q::Value, DepNodeIndex)
where
Q: QueryConfig<Qcx>,
Qcx: QueryContext,
{
if !query.anon() && !query.eval_always() {
// `to_dep_node` is expensive for some `DepKind`s.
let dep_node =
Expand Down