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.

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

Don't enqueue onto a disabled dep_graph. #36973

Merged
merged 1 commit into from
Oct 20, 2016
Merged
Show file tree
Hide file tree
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
19 changes: 8 additions & 11 deletions src/librustc/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,22 +51,15 @@ impl DepGraph {
}
}

/// True if we are actually building a dep-graph. If this returns false,
/// then the other methods on this `DepGraph` will have no net effect.
#[inline]
pub fn enabled(&self) -> bool {
self.data.thread.enabled()
}

pub fn query(&self) -> DepGraphQuery<DefId> {
self.data.thread.query()
}

pub fn in_ignore<'graph>(&'graph self) -> raii::IgnoreTask<'graph> {
pub fn in_ignore<'graph>(&'graph self) -> Option<raii::IgnoreTask<'graph>> {
raii::IgnoreTask::new(&self.data.thread)
}

pub fn in_task<'graph>(&'graph self, key: DepNode<DefId>) -> raii::DepTask<'graph> {
pub fn in_task<'graph>(&'graph self, key: DepNode<DefId>) -> Option<raii::DepTask<'graph>> {
raii::DepTask::new(&self.data.thread, key)
}

Expand All @@ -85,11 +78,15 @@ impl DepGraph {
}

pub fn read(&self, v: DepNode<DefId>) {
self.data.thread.enqueue(DepMessage::Read(v));
if self.data.thread.is_enqueue_enabled() {
self.data.thread.enqueue(DepMessage::Read(v));
}
}

pub fn write(&self, v: DepNode<DefId>) {
self.data.thread.enqueue(DepMessage::Write(v));
if self.data.thread.is_enqueue_enabled() {
self.data.thread.enqueue(DepMessage::Write(v));
}
}

/// Indicates that a previous work product exists for `v`. This is
Expand Down
28 changes: 20 additions & 8 deletions src/librustc/dep_graph/raii.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
DepTask { data: data, key: Some(key) }
-> Option<DepTask<'graph>> {
if data.is_enqueue_enabled() {
Copy link
Contributor

@nikomatsakis nikomatsakis Oct 11, 2016

Choose a reason for hiding this comment

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

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.)

data.enqueue(DepMessage::PushTask(key.clone()));
Some(DepTask { data: data, key: Some(key) })
} else {
None
}
}
}

impl<'graph> Drop for DepTask<'graph> {
fn drop(&mut self) {
self.data.enqueue(DepMessage::PopTask(self.key.take().unwrap()));
if self.data.is_enqueue_enabled() {
self.data.enqueue(DepMessage::PopTask(self.key.take().unwrap()));
}
}
}

Expand All @@ -36,15 +42,21 @@ pub struct IgnoreTask<'graph> {
}

impl<'graph> IgnoreTask<'graph> {
pub fn new(data: &'graph DepGraphThreadData) -> IgnoreTask<'graph> {
data.enqueue(DepMessage::PushIgnore);
IgnoreTask { data: data }
pub fn new(data: &'graph DepGraphThreadData) -> Option<IgnoreTask<'graph>> {
if data.is_enqueue_enabled() {
data.enqueue(DepMessage::PushIgnore);
Some(IgnoreTask { data: data })
} else {
None
}
}
}

impl<'graph> Drop for IgnoreTask<'graph> {
fn drop(&mut self) {
self.data.enqueue(DepMessage::PopIgnore);
if self.data.is_enqueue_enabled() {
self.data.enqueue(DepMessage::PopIgnore);
}
}
}

5 changes: 5 additions & 0 deletions src/librustc/dep_graph/shadow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ impl ShadowGraph {
}
}

#[inline]
pub fn enabled(&self) -> bool {
ENABLED
}

pub fn enqueue(&self, message: &DepMessage) {
if ENABLED {
match self.stack.borrow_state() {
Expand Down
19 changes: 14 additions & 5 deletions src/librustc/dep_graph/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,24 @@ impl DepGraphThreadData {
}
}

/// True if we are actually building the full dep-graph.
#[inline]
pub fn enabled(&self) -> bool {
pub fn is_fully_enabled(&self) -> bool {
self.enabled
}

/// True if (a) we are actually building the full dep-graph, or (b) we are
/// only enqueuing messages in order to sanity-check them (which happens
/// when debug assertions are enabled).
#[inline]
pub fn is_enqueue_enabled(&self) -> bool {
self.is_fully_enabled() || self.shadow_graph.enabled()
}

/// Sends the current batch of messages to the thread. Installs a
/// new vector of messages.
fn swap(&self) {
assert!(self.enabled, "should never swap if not enabled");
assert!(self.is_fully_enabled(), "should never swap if not fully enabled");

// should be a buffer waiting for us (though of course we may
// have to wait for depgraph thread to finish processing the
Expand All @@ -112,7 +121,7 @@ impl DepGraphThreadData {
}

pub fn query(&self) -> DepGraphQuery<DefId> {
assert!(self.enabled, "cannot query if dep graph construction not enabled");
assert!(self.is_fully_enabled(), "should never query if not fully enabled");
self.enqueue(DepMessage::Query);
self.swap();
self.query_in.recv().unwrap()
Expand All @@ -122,9 +131,9 @@ impl DepGraphThreadData {
/// the buffer is full, this may swap.)
#[inline]
pub fn enqueue(&self, message: DepMessage) {
assert!(self.is_enqueue_enabled(), "should never enqueue if not enqueue-enabled");
self.shadow_graph.enqueue(&message);

if self.enabled {
if self.is_fully_enabled() {
self.enqueue_enabled(message);
}
}
Expand Down