Commit f2faee85 authored by Clemens Backes's avatar Clemens Backes Committed by V8 LUCI CQ

[wasm] Use std::shared_mutex instead of base::SharedMutex

base::SharedMutex was implemented as an exclusive lock on Mac, because
of an OS issue on Mac (see https://crbug.com/v8/12037).
https://crrev.com/c/3855361 then introduced a custom implementation on
Mac, which caused performance regressions (see
https://crbug.com/1358856).

Since we rely on C++17 now, we can instead just use {std::shared_mutex},
which does not seem to have the deadlock issue of {pthread_rwlock_t}.
As a smoke test (and to check whether this actually fixes the
performance regressions), only switch one mutex in Wasm compilation to
std::shared_mutex. If this CL looks good, then other places can be
switched over as well.

R=ishell@chromium.org

Bug: chromium:1358856, v8:13256
Change-Id: Ia56efcb7747f191cc3ed7a381096c8f57142aff8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3868954
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: 's avatarIgor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82916}
parent 5a318a23
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#include <algorithm> #include <algorithm>
#include <queue> #include <queue>
#include <shared_mutex>
#include "src/api/api-inl.h" #include "src/api/api-inl.h"
#include "src/base/enum-set.h" #include "src/base/enum-set.h"
...@@ -136,14 +137,14 @@ class CompilationUnitQueues { ...@@ -136,14 +137,14 @@ class CompilationUnitQueues {
Queue* GetQueueForTask(int task_id) { Queue* GetQueueForTask(int task_id) {
int required_queues = task_id + 1; int required_queues = task_id + 1;
{ {
base::SharedMutexGuard<base::kShared> queues_guard(&queues_mutex_); std::shared_lock<std::shared_mutex> queues_guard{queues_mutex_};
if (V8_LIKELY(static_cast<int>(queues_.size()) >= required_queues)) { if (V8_LIKELY(static_cast<int>(queues_.size()) >= required_queues)) {
return queues_[task_id].get(); return queues_[task_id].get();
} }
} }
// Otherwise increase the number of queues. // Otherwise increase the number of queues.
base::SharedMutexGuard<base::kExclusive> queues_guard(&queues_mutex_); std::unique_lock<std::shared_mutex> queues_guard{queues_mutex_};
int num_queues = static_cast<int>(queues_.size()); int num_queues = static_cast<int>(queues_.size());
while (num_queues < required_queues) { while (num_queues < required_queues) {
int steal_from = num_queues + 1; int steal_from = num_queues + 1;
...@@ -198,7 +199,7 @@ class CompilationUnitQueues { ...@@ -198,7 +199,7 @@ class CompilationUnitQueues {
QueueImpl* queue; QueueImpl* queue;
{ {
int queue_to_add = next_queue_to_add.load(std::memory_order_relaxed); int queue_to_add = next_queue_to_add.load(std::memory_order_relaxed);
base::SharedMutexGuard<base::kShared> queues_guard(&queues_mutex_); std::shared_lock<std::shared_mutex> queues_guard{queues_mutex_};
while (!next_queue_to_add.compare_exchange_weak( while (!next_queue_to_add.compare_exchange_weak(
queue_to_add, next_task_id(queue_to_add, queues_.size()), queue_to_add, next_task_id(queue_to_add, queues_.size()),
std::memory_order_relaxed)) { std::memory_order_relaxed)) {
...@@ -232,7 +233,7 @@ class CompilationUnitQueues { ...@@ -232,7 +233,7 @@ class CompilationUnitQueues {
} }
void AddTopTierPriorityUnit(WasmCompilationUnit unit, size_t priority) { void AddTopTierPriorityUnit(WasmCompilationUnit unit, size_t priority) {
base::SharedMutexGuard<base::kShared> queues_guard(&queues_mutex_); std::shared_lock<std::shared_mutex> queues_guard{queues_mutex_};
// Add to the individual queues in a round-robin fashion. No special care is // Add to the individual queues in a round-robin fashion. No special care is
// taken to balance them; they will be balanced by work stealing. // taken to balance them; they will be balanced by work stealing.
// Priorities should only be seen as a hint here; without balancing, we // Priorities should only be seen as a hint here; without balancing, we
...@@ -380,7 +381,7 @@ class CompilationUnitQueues { ...@@ -380,7 +381,7 @@ class CompilationUnitQueues {
// Try to steal from all other queues. If this succeeds, return one of the // Try to steal from all other queues. If this succeeds, return one of the
// stolen units. // stolen units.
{ {
base::SharedMutexGuard<base::kShared> guard(&queues_mutex_); std::shared_lock<std::shared_mutex> guard{queues_mutex_};
for (size_t steal_trials = 0; steal_trials < queues_.size(); for (size_t steal_trials = 0; steal_trials < queues_.size();
++steal_trials, ++steal_task_id) { ++steal_trials, ++steal_task_id) {
if (steal_task_id >= static_cast<int>(queues_.size())) { if (steal_task_id >= static_cast<int>(queues_.size())) {
...@@ -437,7 +438,7 @@ class CompilationUnitQueues { ...@@ -437,7 +438,7 @@ class CompilationUnitQueues {
// Try to steal from all other queues. If this succeeds, return one of the // Try to steal from all other queues. If this succeeds, return one of the
// stolen units. // stolen units.
{ {
base::SharedMutexGuard<base::kShared> guard(&queues_mutex_); std::shared_lock<std::shared_mutex> guard{queues_mutex_};
for (size_t steal_trials = 0; steal_trials < queues_.size(); for (size_t steal_trials = 0; steal_trials < queues_.size();
++steal_trials, ++steal_task_id) { ++steal_trials, ++steal_task_id) {
if (steal_task_id >= static_cast<int>(queues_.size())) { if (steal_task_id >= static_cast<int>(queues_.size())) {
...@@ -512,7 +513,7 @@ class CompilationUnitQueues { ...@@ -512,7 +513,7 @@ class CompilationUnitQueues {
} }
// {queues_mutex_} protectes {queues_}; // {queues_mutex_} protectes {queues_};
base::SharedMutex queues_mutex_; std::shared_mutex queues_mutex_;
std::vector<std::unique_ptr<QueueImpl>> queues_; std::vector<std::unique_ptr<QueueImpl>> queues_;
const int num_declared_functions_; const int num_declared_functions_;
......
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