-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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.
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
Conversation
@bors try |
Clean up and optimize OpenTask / read_index r? @michaelwoerister
☀️ Test successful - status-travis |
@rust-timer build a0a3e1c |
Success: Queued a0a3e1c with parent ad781a0, comparison URL. |
Finished benchmarking try commit a0a3e1c |
@@ -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); |
There was a problem hiding this comment.
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.
☔ The latest upstream changes (presumably #57061) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this 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+
@michaelwoerister I think bors doesn't work with commands in review comments |
@bors r=michaelwoerister |
📌 Commit 584a520 has been approved by |
Thanks for the heads up! |
Clean up and optimize OpenTask / read_index r? @michaelwoerister
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister