Skip to content

Clean up and optimize OpenTask / read_index #57114

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2019
Merged

Clean up and optimize OpenTask / read_index #57114

merged 1 commit into from
Jan 8, 2019

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Dec 25, 2018

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

Zoxc commented Dec 25, 2018

@bors try

@bors
Copy link
Collaborator

bors commented Dec 25, 2018

⌛ Trying commit 8c1fab1 with merge a0a3e1c...

bors added a commit that referenced this pull request Dec 25, 2018
@bors
Auto merge of #57114 - Zoxc:query-perf11, r=<try>
a0a3e1c 
Clean up and optimize OpenTask / read_index

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Dec 25, 2018

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

@Zoxc
Copy link
Contributor Author

Zoxc commented Dec 25, 2018

@rust-timer build a0a3e1c

@rust-timer
Copy link
Collaborator

Success: Queued a0a3e1c with parent ad781a0, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit a0a3e1c

Zoxc
Zoxc commented Dec 25, 2018
View reviewed changes
src/librustc/dep_graph/graph.rs
@@ -409,7 +418,7 @@ impl DepGraph {
#[inline]
pub fn read_index(&self, dep_node_index: DepNodeIndex) {
if let Some(ref data) = self.data {
data.current.borrow_mut().read_index(dep_node_index);
data.read_index(dep_node_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.

Note: We no longer need the global dep graph lock here for parallel queries.

@bors
Copy link
Collaborator

bors commented Dec 31, 2018

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

michaelwoerister
michaelwoerister approved these changes Jan 7, 2019
View reviewed changes
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.

Looks great, thanks @Zoxc!

I wonder if we can get rid of the lock around the read-set too at some point. Maybe collect reads per thread, without locking, and merge them during task completion. But correctness is a concern here. We rely on the order of reads during try_mark_green. Anyway, no need to solve this now.

@bors r+

@phansch
Copy link
Member

phansch commented Jan 7, 2019

@michaelwoerister I think bors doesn't work with commands in review comments

@Mark-Simulacrum
Copy link
Member

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Jan 7, 2019

📌 Commit 584a520 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 7, 2019
@michaelwoerister
Copy link
Member

Thanks for the heads up!

@bors
Copy link
Collaborator

bors commented Jan 8, 2019

⌛ Testing commit 584a520 with merge b8c8f0b...

bors added a commit that referenced this pull request Jan 8, 2019
@bors
Auto merge of #57114 - Zoxc:query-perf11, r=michaelwoerister
b8c8f0b 
Clean up and optimize OpenTask / read_index

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Jan 8, 2019

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

@bors bors merged commit 584a520 into rust-lang:master Jan 8, 2019
@Zoxc Zoxc deleted the query-perf11 branch January 8, 2019 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@michaelwoerister michaelwoerister michaelwoerister approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Zoxc @bors @rust-timer @phansch @Mark-Simulacrum @michaelwoerister @rust-highfive