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

Projects
None yet
5 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 22, 2018

Blocked on #56614

r? @michaelwoerister

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 22, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2018

⌛️ Trying commit 84aa34d with merge 2897f68...

bors added a commit that referenced this pull request Dec 22, 2018

Auto merge of #57065 - Zoxc:graph-tweaks, r=<try>
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on #56614

r? @michaelwoerister
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2018

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

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 22, 2018

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 2897f68

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 23, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 23, 2018

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 23, 2018

Finished benchmarking try commit 2897f68

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jan 7, 2019

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

@Zoxc

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jan 8, 2019

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

@Zoxc Zoxc force-pushed the Zoxc:graph-tweaks branch from 84aa34d to 1bfc43f Jan 9, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 15, 2019

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

@Zoxc Zoxc force-pushed the Zoxc:graph-tweaks branch from 1bfc43f to 8e88833 Jan 15, 2019

@michaelwoerister
Copy link
Contributor

michaelwoerister left a comment

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>(

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 15, 2019

Contributor

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());

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 15, 2019

Contributor

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.

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 15, 2019

Contributor

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

@Zoxc Zoxc force-pushed the Zoxc:graph-tweaks branch from 8e88833 to 1313678 Jan 15, 2019

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Jan 15, 2019

@bors r=michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 15, 2019

📌 Commit 1313678 has been approved by michaelwoerister

Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019

Rollup merge of rust-lang#57065 - Zoxc:graph-tweaks, r=michaelwoerister
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

Auto merge of #57637 - Centril:rollup, r=Centril
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

Rollup merge of rust-lang#57065 - Zoxc:graph-tweaks, r=michaelwoerister
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

Rollup merge of rust-lang#57065 - Zoxc:graph-tweaks, r=michaelwoerister
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

Rollup merge of rust-lang#57065 - Zoxc:graph-tweaks, r=michaelwoerister
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 17, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #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])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 17, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #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])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #57690 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #56594 (Remove confusing comment about ideally using `!` for `c_void`)
 - #56996 (Move spin_loop_hint to core::hint module)
 - #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])
 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57340 (Use correct tracking issue for c_variadic)
 - #57357 (Cleanup PartialEq docs.)
 - #57370 (Support passing cflags/cxxflags/ldflags to LLVM build)
 - #57501 (High priority resolutions for associated variants)
 - #57551 (resolve: Add a test for issue #57539)
 - #57610 (Fix nested `?` matchers)
 - #57635 (use structured macro and path resolve suggestions)
 - #57636 (Fix sources sidebar not showing up)
 - #57646 (Fixes text becoming invisible when element targetted)
 - #57654 (Add some links in std::fs.)
 - #57655 (OSX: fix #57534 registering thread dtors while running thread dtors)
 - #57659 (Fix release manifest generation)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

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

Auto merge of #57065 - Zoxc:graph-tweaks, r=michaelwoerister
Optimize try_mark_green and eliminate the lock on dep node colors

Blocked on #56614

r? @michaelwoerister
@bors

This comment has been minimized.

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

1 check passed

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