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

Optimize try_mark_green and eliminate the lock on dep node colors #57065

Merged
merged 2 commits into from
Jan 18, 2019

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Dec 22, 2018

Blocked on #56614

r? @michaelwoerister

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 22, 2018

@bors try

@bors
Copy link
Contributor

bors commented Dec 22, 2018

⌛ Trying commit 84aa34d621f10826957bb64c08699307a3f69fc3 with merge 2897f68c5b419b38ac5e1a32910abe9112ea8a3c...

@bors
Copy link
Contributor

bors commented Dec 22, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member

Nice :) This is something I wanted to try out for a while. (I haven't looked at the specific implementation here yet though).

@rust-timer build 2897f68c5b419b38ac5e1a32910abe9112ea8a3c

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 23, 2018

@rust-timer build 2897f68c5b419b38ac5e1a32910abe9112ea8a3c

@rust-timer
Copy link
Collaborator

Success: Queued 2897f68c5b419b38ac5e1a32910abe9112ea8a3c with parent fa922ab, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 2897f68c5b419b38ac5e1a32910abe9112ea8a3c

@michaelwoerister
Copy link
Member

This is another case where measuring with parallel queries would be useful...

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 7, 2019

@michaelwoerister Benchmarks on x86 won't be useful, since the atomic operations lowers to regular load/stores, so this literally just removes the lock on dep node colors.

@michaelwoerister
Copy link
Member

I know but that lock can only be really expensive with parallel queries because of contention, right?

@bors
Copy link
Contributor

bors commented Jan 15, 2019

☔ The latest upstream changes (presumably #57607) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

It would have been nice to have a performance comparison for the multi-threaded case but that's complicated and not really worth the effort. The changes already show a small win in single-threaded mode and it can only improve the situation with parallel queries, so let's go ahead and merge.

r=me with the nits addressed.

Great work, @Zoxc!


debug_assert!(data.colors.borrow().get(prev_dep_node_index).is_none());
fn try_mark_previous_green<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a doc comment here? Something like "Try to mark a dep-node green for which we already know that it existed in the previous compilation session".


// We never try to mark inputs as green
// FIXME: Make an debug_assert!
assert!(!dep_node.kind.is_input());
Copy link
Member

Choose a reason for hiding this comment

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

We already do the debug assertion a few lines above. You can just remove this one (and move the comment up).

// This DepNode and the corresponding query invocation existed
// in the previous compilation session too, so we can try to
// mark it as green by recursively marking all of its
// dependencies green.
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this comment into the None branch of the match below?

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 15, 2019

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 15, 2019

📌 Commit 1313678 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on rust-lang#56614

r? @michaelwoerister
bors added a commit that referenced this pull request Jan 16, 2019
Rollup of 6 pull requests

Successful merges:

 - #56884 (rustdoc: overhaul code block lexing errors)
 - #57065 (Optimize try_mark_green and eliminate the lock on dep node colors)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57551 (resolve: Add a test for issue #57539)
 - #57598 (Add missing unpretty option help message)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on rust-lang#56614

r? @michaelwoerister
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on rust-lang#56614

r? @michaelwoerister
Centril added a commit to Centril/rust that referenced this pull request Jan 17, 2019
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on rust-lang#56614

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Jan 18, 2019

⌛ Testing commit 1313678 with merge 527b8d4...

bors added a commit that referenced this pull request Jan 18, 2019
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on #56614

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Jan 18, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: michaelwoerister
Pushing 527b8d4 to master...

@bors bors merged commit 1313678 into rust-lang:master Jan 18, 2019
@Zoxc Zoxc deleted the graph-tweaks branch January 28, 2019 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants