Commit 0900e72b authored by Etienne Pierre-doray's avatar Etienne Pierre-doray Committed by Commit Bot

[Jobs]: Fix AcquireTaskIds memory fences.

This reflects the change made in chrome:
https://chromium-review.googlesource.com/c/chromium/src/+/2387554

I somehow thoughts that DefaultJob didn't need the fence, but
TSAN detected the same kind of failures after
9e8c54f8 started using AcquireTaskId.

Drive-by: move delegate outside the loop in Join() to avoid releasing
the task_id many times.

Change-Id: I2ab6bf1bd3eeb7a66e39f20a7e0aa61a9c1ebc44
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2401964Reviewed-by: 's avatarUlan Degenbaev <ulan@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69818}
parent 36138aff
...@@ -70,19 +70,24 @@ uint8_t DefaultJobState::AcquireTaskId() { ...@@ -70,19 +70,24 @@ uint8_t DefaultJobState::AcquireTaskId() {
kMaxWorkersPerJob); kMaxWorkersPerJob);
uint32_t new_assigned_task_ids = 0; uint32_t new_assigned_task_ids = 0;
uint8_t task_id = 0; uint8_t task_id = 0;
// memory_order_acquire on success, matched with memory_order_release in
// ReleaseTaskId() so that operations done by previous threads that had
// the same task_id become visible to the current thread.
do { do {
// Count trailing one bits. This is the id of the right-most 0-bit in // Count trailing one bits. This is the id of the right-most 0-bit in
// |assigned_task_ids|. // |assigned_task_ids|.
task_id = v8::base::bits::CountTrailingZeros32(~assigned_task_ids); task_id = v8::base::bits::CountTrailingZeros32(~assigned_task_ids);
new_assigned_task_ids = assigned_task_ids | (uint32_t(1) << task_id); new_assigned_task_ids = assigned_task_ids | (uint32_t(1) << task_id);
} while (!assigned_task_ids_.compare_exchange_weak( } while (!assigned_task_ids_.compare_exchange_weak(
assigned_task_ids, new_assigned_task_ids, std::memory_order_relaxed)); assigned_task_ids, new_assigned_task_ids, std::memory_order_acquire,
std::memory_order_relaxed));
return task_id; return task_id;
} }
void DefaultJobState::ReleaseTaskId(uint8_t task_id) { void DefaultJobState::ReleaseTaskId(uint8_t task_id) {
uint32_t previous_task_ids = // memory_order_release to match AcquireTaskId().
assigned_task_ids_.fetch_and(~(uint32_t(1) << task_id)); uint32_t previous_task_ids = assigned_task_ids_.fetch_and(
~(uint32_t(1) << task_id), std::memory_order_release);
DCHECK(previous_task_ids & (uint32_t(1) << task_id)); DCHECK(previous_task_ids & (uint32_t(1) << task_id));
USE(previous_task_ids); USE(previous_task_ids);
} }
...@@ -99,8 +104,8 @@ void DefaultJobState::Join() { ...@@ -99,8 +104,8 @@ void DefaultJobState::Join() {
++active_workers_; ++active_workers_;
can_run = WaitForParticipationOpportunityLockRequired(); can_run = WaitForParticipationOpportunityLockRequired();
} }
DefaultJobState::JobDelegate delegate(this);
while (can_run) { while (can_run) {
DefaultJobState::JobDelegate delegate(this);
job_task_->Run(&delegate); job_task_->Run(&delegate);
base::MutexGuard guard(&mutex_); base::MutexGuard guard(&mutex_);
can_run = WaitForParticipationOpportunityLockRequired(); can_run = WaitForParticipationOpportunityLockRequired();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment