Don't enqueue onto a disabled dep_graph. #36973

Merged
merged 1 commit into from Oct 20, 2016

Conversation

Projects
None yet
6 participants
@nnethercote
Contributor

nnethercote commented Oct 5, 2016

This commit guards all calls to DepGraphThreadData::enqueue with a
check to make sure it is enabled. This avoids some useless allocation
and vector manipulations when it is disabled (i.e. when incremental
compilation is off) which improves speed by 1--2% on most of the
rustc-benchmarks.

This commit has an observable functional change: when the dep_graph is
disabled its shadow_graph will no longer receive messages. This should
be ok because these message are only used when debug assertions are
enabled.

r? @nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 5, 2016

Contributor

This should be ok because these message are only used when debug assertions are
enabled.

I used to have this setup, but I changed it to the current one specifically so that the shadow graph was active even if incremental is disabled. This is because it detects common errors that are otherwise overlooked (at least until people are using incremental by default).

Perhaps we could do this setup, but only if debug-assertions are not enabled?

Contributor

nikomatsakis commented Oct 5, 2016

This should be ok because these message are only used when debug assertions are
enabled.

I used to have this setup, but I changed it to the current one specifically so that the shadow graph was active even if incremental is disabled. This is because it detects common errors that are otherwise overlooked (at least until people are using incremental by default).

Perhaps we could do this setup, but only if debug-assertions are not enabled?

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Oct 5, 2016

Contributor

FYI: As far as I know, debug assertions are always enabled on the build bots.

Contributor

michaelwoerister commented Oct 5, 2016

FYI: As far as I know, debug assertions are always enabled on the build bots.

@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Oct 5, 2016

Contributor

@michaelwoerister: I think it's the exact opposite (at least on the stable/beta/nightly build bots): debug_assertions are never enabled (which is why DEBUG logging does not work in these builds)

Contributor

TimNN commented Oct 5, 2016

@michaelwoerister: I think it's the exact opposite (at least on the stable/beta/nightly build bots): debug_assertions are never enabled (which is why DEBUG logging does not work in these builds)

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Oct 5, 2016

Contributor

@TimNN I might not be using the right terminology. What I meant was that the tests that gate whether a PR gets merged are run using a compiler where debug assertions are enabled.

Contributor

michaelwoerister commented Oct 5, 2016

@TimNN I might not be using the right terminology. What I meant was that the tests that gate whether a PR gets merged are run using a compiler where debug assertions are enabled.

@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Oct 5, 2016

Contributor

Ah well, I suppose those are build bots as well. (Not sure if there is an "official" terminology).

(And I just checked, debug_assertions are indeed enabled on the test bots (or whatever you want to call them): https://github.com/rust-lang/rust-buildbot/blob/master/master/master.cfg#L1775)

Contributor

TimNN commented Oct 5, 2016

Ah well, I suppose those are build bots as well. (Not sure if there is an "official" terminology).

(And I just checked, debug_assertions are indeed enabled on the test bots (or whatever you want to call them): https://github.com/rust-lang/rust-buildbot/blob/master/master/master.cfg#L1775)

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Oct 5, 2016

Contributor

@TimNN Thanks for making facts out of my speculations :)

Contributor

michaelwoerister commented Oct 5, 2016

@TimNN Thanks for making facts out of my speculations :)

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 5, 2016

Contributor

Ok, I will update the code to still push the messages if debug assertions are enabled.

Contributor

nnethercote commented Oct 5, 2016

Ok, I will update the code to still push the messages if debug assertions are enabled.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
Contributor

nnethercote commented Oct 6, 2016

@@ -19,15 +19,21 @@ pub struct DepTask<'graph> {
impl<'graph> DepTask<'graph> {
pub fn new(data: &'graph DepGraphThreadData, key: DepNode<DefId>)
- -> DepTask<'graph> {
- data.enqueue(DepMessage::PushTask(key.clone()));

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 11, 2016

Contributor

Have you measured the impact of this on the perf also when construction is enabled? When I first wrote the code, I went to some lengths to make it straightline and cheap when the graph was enabled, and the overhead I measured was quite low. The idea was that copying into the buffer without a branch could actually be cheaper than not copying at all. I did measure and I recall finding this to be the case (I think I tested libsyntax), but a lot has changed since then, and for all I know my measurements at the time were flawed. =) We have a better setup now.

(After all, we do expect "incremental enabled" to be the common case, right? I would think that incr will always be on, unless you are building a final release artifact, in which case you are already asking for a lot of time to be spent on optimization.)

@nikomatsakis

nikomatsakis Oct 11, 2016

Contributor

Have you measured the impact of this on the perf also when construction is enabled? When I first wrote the code, I went to some lengths to make it straightline and cheap when the graph was enabled, and the overhead I measured was quite low. The idea was that copying into the buffer without a branch could actually be cheaper than not copying at all. I did measure and I recall finding this to be the case (I think I tested libsyntax), but a lot has changed since then, and for all I know my measurements at the time were flawed. =) We have a better setup now.

(After all, we do expect "incremental enabled" to be the common case, right? I would think that incr will always be on, unless you are building a final release artifact, in which case you are already asking for a lot of time to be spent on optimization.)

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 11, 2016

Contributor

The only measurement of incremental compilation I did is of syntex-incr in rustc-benchmarks. Its speed was unaffected by the patch. Perfectly predictable branches tend to be cheap :)

Contributor

nnethercote commented Oct 11, 2016

The only measurement of incremental compilation I did is of syntex-incr in rustc-benchmarks. Its speed was unaffected by the patch. Perfectly predictable branches tend to be cheap :)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 11, 2016

Contributor

Seems good.

@bors r+

Contributor

nikomatsakis commented Oct 11, 2016

Seems good.

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 11, 2016

Contributor

📌 Commit d54b044 has been approved by nikomatsakis

Contributor

bors commented Oct 11, 2016

📌 Commit d54b044 has been approved by nikomatsakis

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 12, 2016

Rollup merge of #36973 - nnethercote:dep_graph, r=nikomatsakis
Don't enqueue onto a disabled dep_graph.

This commit guards all calls to `DepGraphThreadData::enqueue` with a
check to make sure it is enabled. This avoids some useless allocation
and vector manipulations when it is disabled (i.e. when incremental
compilation is off) which improves speed by 1--2% on most of the
rustc-benchmarks.

This commit has an observable functional change: when the dep_graph is
disabled its `shadow_graph` will no longer receive messages. This should
be ok because these message are only used when debug assertions are
enabled.

r? @nikomatsakis

bors added a commit that referenced this pull request Oct 12, 2016

bors added a commit that referenced this pull request Oct 12, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 13, 2016

Member

@bors: r-

Unfortunately I think this caused this failure, specifically:

Building stage1 std artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
    Finished release [optimized] target(s) in 656.99 secs
   Compiling build_helper v0.1.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/build_helper)
   Compiling core v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/libcore)
   Compiling gcc v0.3.35
   Compiling unwind v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/libunwind)
   Compiling libc v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/rustc/libc_shim)
Build failed, waiting for other jobs to finish...
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

thread 'rustc' panicked at 'should never swap if not fully enabled', C:\bot\slave\auto-win-msvc-64-cargotest\build\src\librustc\dep_graph\thread.rs:108
note: Run with `RUST_BACKTRACE=1` for a backtrace.

This happened on MSVC for me but not on Linux, and I had to take it out of a rollup as a result.

Member

alexcrichton commented Oct 13, 2016

@bors: r-

Unfortunately I think this caused this failure, specifically:

Building stage1 std artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
    Finished release [optimized] target(s) in 656.99 secs
   Compiling build_helper v0.1.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/build_helper)
   Compiling core v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/libcore)
   Compiling gcc v0.3.35
   Compiling unwind v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/libunwind)
   Compiling libc v0.0.0 (file:///C:/bot/slave/auto-win-msvc-64-cargotest/build/src/rustc/libc_shim)
Build failed, waiting for other jobs to finish...
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

thread 'rustc' panicked at 'should never swap if not fully enabled', C:\bot\slave\auto-win-msvc-64-cargotest\build\src\librustc\dep_graph\thread.rs:108
note: Run with `RUST_BACKTRACE=1` for a backtrace.

This happened on MSVC for me but not on Linux, and I had to take it out of a rollup as a result.

Don't enqueue onto a disabled dep_graph.
This commit guards all calls to `DepGraphThreadData::enqueue` with a
check to make sure it is enabled. This requires distinguishing between a
"fully enabled" and an "enqueue-enabled" graph.

This change avoids some useless allocation and vector manipulations when
the graph is disabled (i.e. when incremental compilation is off) which
improves speed by ~1% on some of the rustc-benchmarks.
@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 18, 2016

Contributor

I've fixed the problem and pushed an updated commit. The problem was that I mistakenly removed the the enabled check in enqueue (right at the end of the diff). It's back now.

r? @nikomatsakis

Contributor

nnethercote commented Oct 18, 2016

I've fixed the problem and pushed an updated commit. The problem was that I mistakenly removed the the enabled check in enqueue (right at the end of the diff). It's back now.

r? @nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
Contributor

nikomatsakis commented Oct 19, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 19, 2016

Contributor

📌 Commit cde42cd has been approved by nikomatsakis

Contributor

bors commented Oct 19, 2016

📌 Commit cde42cd has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 20, 2016

Contributor

⌛️ Testing commit cde42cd with merge eb38d42...

Contributor

bors commented Oct 20, 2016

⌛️ Testing commit cde42cd with merge eb38d42...

bors added a commit that referenced this pull request Oct 20, 2016

Auto merge of #36973 - nnethercote:dep_graph, r=nikomatsakis
Don't enqueue onto a disabled dep_graph.

This commit guards all calls to `DepGraphThreadData::enqueue` with a
check to make sure it is enabled. This avoids some useless allocation
and vector manipulations when it is disabled (i.e. when incremental
compilation is off) which improves speed by 1--2% on most of the
rustc-benchmarks.

This commit has an observable functional change: when the dep_graph is
disabled its `shadow_graph` will no longer receive messages. This should
be ok because these message are only used when debug assertions are
enabled.

r? @nikomatsakis

@bors bors merged commit cde42cd into rust-lang:master Oct 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:dep_graph branch Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment