Revert "[graphite] Add multiple work lists to SkExecutor"
This reverts commit 3b83fd3eba3f29d2b0e8b27067f43ba781642a22.
Reason for revert: ASAN complaint
Original change's description:
> [graphite] Add multiple work lists to SkExecutor
>
> Bug: b/434712686
> Change-Id: I5266e19b8da90ea18ee31ea80550875d97498f01
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1050616
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Reviewed-by: Thomas Smith <thomsmit@google.com>
> Commit-Queue: Robert Phillips <robertphillips@google.com>
Bug: b/434712686
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: I8741a527445ea0f4186d74e4747e0c460108d0f7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1056436
Auto-Submit: Robert Phillips <robertphillips@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index b25ec09..4872c58 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -503,7 +503,7 @@
(r'std::mutex', 'SkMutex'),
(r'std::shared_mutex', 'SkSharedMutex'),
(r'std::stop_token', ''),
- (r'std::thread', '', ['^tests/', 'SkExecutor']),
+ (r'std::thread', '', ['^tests/']),
# We used to have separate symbols for this, but coalesced them to make the
# Bazel build easier.
diff --git a/gn/tests.gni b/gn/tests.gni
index a01097b..819e5dd 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -74,7 +74,6 @@
"$_tests/EmptyPathTest.cpp",
"$_tests/EncodeTest.cpp",
"$_tests/EncodedInfoTest.cpp",
- "$_tests/ExecutorTest.cpp",
"$_tests/ExifTest.cpp",
"$_tests/ExtendedSkColorTypeTests.cpp",
"$_tests/F16DrawTest.cpp",
diff --git a/include/core/SkExecutor.h b/include/core/SkExecutor.h
index eb62c15..88e2ca6 100644
--- a/include/core/SkExecutor.h
+++ b/include/core/SkExecutor.h
@@ -22,27 +22,12 @@
static std::unique_ptr<SkExecutor> MakeLIFOThreadPool(int threads = 0,
bool allowBorrowing = true);
- // A work list is the queue or stack to which work is added and removed. The above two
- // factory functions create an executor with only one list while the following two factories
- // can create executors with multiple work lists. Having multiple work lists allows for
- // prioritization with work being pulled from the lower indexed work lists first - with
- // work list '0' being the highest priority.
- static std::unique_ptr<SkExecutor> MakeMultiListFIFOThreadPool(int numWorkLists,
- int threads = 0,
- bool allowBorrowing = true);
- static std::unique_ptr<SkExecutor> MakeMultiListLIFOThreadPool(int numWorkLists,
- int threads = 0,
- bool allowBorrowing = true);
-
// There is always a default SkExecutor available by calling SkExecutor::GetDefault().
static SkExecutor& GetDefault();
static void SetDefault(SkExecutor*); // Does not take ownership. Not thread safe.
// Add work to execute.
- virtual void add(std::function<void(void)>);
- virtual void add(std::function<void(void)>, int workList) = 0;
-
- virtual void discardAllPendingWork() = 0;
+ virtual void add(std::function<void(void)>) = 0;
// If it makes sense for this executor, use this thread to execute work for a little while.
virtual void borrow() {}
diff --git a/src/core/SkExecutor.cpp b/src/core/SkExecutor.cpp
index 484aaa5..4c97f7f 100644
--- a/src/core/SkExecutor.cpp
+++ b/src/core/SkExecutor.cpp
@@ -9,7 +9,6 @@
#include "include/private/base/SkMutex.h"
#include "include/private/base/SkSemaphore.h"
#include "include/private/base/SkTArray.h"
-#include "include/private/base/SkTPin.h"
#include "src/base/SkNoDestructor.h"
#include <deque>
@@ -34,16 +33,11 @@
SkExecutor::~SkExecutor() {}
-void SkExecutor::add(std::function<void(void)> work) {
- this->add(std::move(work), /* workList= */ 0);
-}
-
// The default default SkExecutor is an SkTrivialExecutor, which just runs the work right away.
class SkTrivialExecutor final : public SkExecutor {
- void add(std::function<void(void)> work, int /* workList */) override {
+ void add(std::function<void(void)> work) override {
work();
}
- void discardAllPendingWork() override {}
};
static SkExecutor& trivial_executor() {
@@ -80,12 +74,7 @@
template <typename WorkList>
class SkThreadPool final : public SkExecutor {
public:
- explicit SkThreadPool(int numWorkLists, int threads, bool allowBorrowing)
- : fNumWorkLists(numWorkLists < 1 ? 1 : numWorkLists)
- , fAllowBorrowing(allowBorrowing) {
-
- fWorkLists = std::make_unique<WorkList[]>(fNumWorkLists);
-
+ explicit SkThreadPool(int threads, bool allowBorrowing) : fAllowBorrowing(allowBorrowing) {
for (int i = 0; i < threads; i++) {
fThreads.emplace_back(&Loop, this);
}
@@ -94,8 +83,7 @@
~SkThreadPool() override {
// Signal each thread that it's time to shut down.
for (int i = 0; i < fThreads.size(); i++) {
- // Add the notification to the highest priority list
- this->add(nullptr, /* workList= */ 0);
+ this->add(nullptr);
}
// Wait for each thread to shut down.
for (int i = 0; i < fThreads.size(); i++) {
@@ -103,27 +91,16 @@
}
}
- void add(std::function<void(void)> work, int workList) override {
- workList = SkTPin(workList, 0, fNumWorkLists-1);
-
+ void add(std::function<void(void)> work) override {
// Add some work to our pile of work to do.
{
SkAutoMutexExclusive lock(fWorkLock);
-
- fWorkLists[workList].emplace_back(std::move(work));
+ fWork.emplace_back(std::move(work));
}
// Tell the Loop() threads to pick it up.
fWorkAvailable.signal(1);
}
- void discardAllPendingWork() override {
- SkAutoMutexExclusive lock(fWorkLock);
-
- for (int i = 0; i < fNumWorkLists; ++i) {
- fWorkLists[i].clear();
- }
- }
-
void borrow() override {
// If there is work waiting and we're allowed to borrow work, do it.
if (fAllowBorrowing && fWorkAvailable.try_wait()) {
@@ -132,26 +109,13 @@
}
private:
- // This method should usually be called only when fWorkAvailable indicates there's work to do.
+ // This method should be called only when fWorkAvailable indicates there's work to do.
bool do_work() {
std::function<void(void)> work;
- bool workAvailable = false;
{
SkAutoMutexExclusive lock(fWorkLock);
-
- for (int i = 0; i < fNumWorkLists; ++i) {
- if (!fWorkLists[i].empty()) {
- workAvailable = true;
- work = pop(&fWorkLists[i]);
- break;
- }
- }
- }
-
- if (!workAvailable) {
- // Because we can discard work asynchronous to Loop() we can sometimes get in this
- // method with no work to do
- return true;
+ SkASSERT(!fWork.empty()); // TODO: if (fWork.empty()) { return true; } ?
+ work = pop(&fWork);
}
if (!work) {
@@ -172,40 +136,20 @@
// Both SkMutex and SkSpinlock can work here.
using Lock = SkMutex;
- TArray<std::thread> fThreads;
- const int fNumWorkLists; // guaranteed >= 1
- std::unique_ptr<WorkList[]> fWorkLists SK_GUARDED_BY(fWorkLock);
- Lock fWorkLock;
- SkSemaphore fWorkAvailable;
- const bool fAllowBorrowing;
+ TArray<std::thread> fThreads;
+ WorkList fWork;
+ Lock fWorkLock;
+ SkSemaphore fWorkAvailable;
+ bool fAllowBorrowing;
};
std::unique_ptr<SkExecutor> SkExecutor::MakeFIFOThreadPool(int threads, bool allowBorrowing) {
using WorkList = std::deque<std::function<void(void)>>;
- return std::make_unique<SkThreadPool<WorkList>>(/* numWorkLists= */ 1,
- threads > 0 ? threads : num_cores(),
+ return std::make_unique<SkThreadPool<WorkList>>(threads > 0 ? threads : num_cores(),
allowBorrowing);
}
std::unique_ptr<SkExecutor> SkExecutor::MakeLIFOThreadPool(int threads, bool allowBorrowing) {
using WorkList = TArray<std::function<void(void)>>;
- return std::make_unique<SkThreadPool<WorkList>>(/* numWorkLists= */ 1,
- threads > 0 ? threads : num_cores(),
- allowBorrowing);
-}
-
-std::unique_ptr<SkExecutor> SkExecutor::MakeMultiListFIFOThreadPool(int numWorkLists,
- int threads,
- bool allowBorrowing) {
- using WorkList = std::deque<std::function<void(void)>>;
- return std::make_unique<SkThreadPool<WorkList>>(numWorkLists,
- threads > 0 ? threads : num_cores(),
- allowBorrowing);
-}
-std::unique_ptr<SkExecutor> SkExecutor::MakeMultiListLIFOThreadPool(int numWorkLists,
- int threads,
- bool allowBorrowing) {
- using WorkList = TArray<std::function<void(void)>>;
- return std::make_unique<SkThreadPool<WorkList>>(numWorkLists,
- threads > 0 ? threads : num_cores(),
+ return std::make_unique<SkThreadPool<WorkList>>(threads > 0 ? threads : num_cores(),
allowBorrowing);
}
diff --git a/src/core/SkTaskGroup.cpp b/src/core/SkTaskGroup.cpp
index 3303c3a..cd5c50f 100644
--- a/src/core/SkTaskGroup.cpp
+++ b/src/core/SkTaskGroup.cpp
@@ -14,20 +14,11 @@
SkTaskGroup::SkTaskGroup(SkExecutor& executor) : fPending(0), fExecutor(executor) {}
void SkTaskGroup::add(std::function<void(void)> fn) {
- this->add(std::move(fn), /* workList= */ 0);
-}
-
-void SkTaskGroup::add(std::function<void(void)> fn, int workList) {
fPending.fetch_add(+1, std::memory_order_relaxed);
fExecutor.add([this, fn{std::move(fn)}] {
- fn();
- fPending.fetch_add(-1, std::memory_order_release);
- },
- workList);
-}
-
-void SkTaskGroup::discardAllPendingWork() {
- fExecutor.discardAllPendingWork();
+ fn();
+ fPending.fetch_add(-1, std::memory_order_release);
+ });
}
void SkTaskGroup::batch(int N, std::function<void(int)> fn) {
diff --git a/src/core/SkTaskGroup.h b/src/core/SkTaskGroup.h
index 5f1b910..7e9d2ba 100644
--- a/src/core/SkTaskGroup.h
+++ b/src/core/SkTaskGroup.h
@@ -25,9 +25,6 @@
// Add a task to this SkTaskGroup.
void add(std::function<void(void)> fn);
- void add(std::function<void(void)> fn, int workList);
-
- void discardAllPendingWork();
// Add a batch of N tasks, all calling fn with different arguments.
void batch(int N, std::function<void(int)> fn);
diff --git a/tests/ExecutorTest.cpp b/tests/ExecutorTest.cpp
deleted file mode 100644
index 8ffe433..0000000
--- a/tests/ExecutorTest.cpp
+++ /dev/null
@@ -1,152 +0,0 @@
-/*
- * Copyright 2025 Google LLC
- *
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-#include "include/core/SkExecutor.h"
-#include "src/base/SkSpinlock.h"
-#include "src/core/SkTaskGroup.h"
-#include "tests/Test.h"
-
-#include <thread>
-
-namespace {
-
-constexpr int kNumWorkLists = 2;
-constexpr int kNumThreads = 4;
-constexpr int kHighPriority = 0;
-constexpr int kLowPriority = 1;
-
-// This utility just collects a set kMaxCount ints in a thread-safe manner. It has a
-// predicate to check if the results match expectations.
-class Collector {
-public:
- static constexpr int kMaxCount = 200;
-
- void insert(int i) SK_EXCLUDES(fSpinLock) {
- SkAutoSpinlock lock{fSpinLock};
-
- if (fCount >= kMaxCount) {
- return;
- }
-
- fData[fCount++] = i;
- }
-
- int count() const SK_EXCLUDES(fSpinLock) {
- SkAutoSpinlock lock{fSpinLock};
- return fCount;
- }
-
-#if defined(SK_DEBUG)
- void dump() const SK_EXCLUDES(fSpinLock) {
- SkAutoSpinlock lock{fSpinLock};
-
- for (int i = 0; i < fCount; ++i) {
- SkDebugf("%d ", fData[i]);
- }
- SkDebugf("\n");
- }
-#endif
-
- // For this following tests we expect that all the high priority work is completed
- // before the low priority work. The length of the two task types is set up so,
- // even if low priority work is picked up while the high priority tasks are finishing,
- // all the high priority task should still finish before the low priority ones.
- bool dataIsInOrder() const SK_EXCLUDES(fSpinLock) {
- SkAutoSpinlock lock{fSpinLock};
-
- for (int i = 0; i < fCount; ++i) {
- if (i < 100) {
- if (fData[i] >= 100) { return false; }
- } else {
- if (fData[i] < 100) { return false; }
- }
- }
- return true;
- }
-
-private:
- mutable SkSpinlock fSpinLock;
-
- int fCount SK_GUARDED_BY(fSpinLock) = 0;
- int fData[kMaxCount] SK_GUARDED_BY(fSpinLock);
-};
-
-// Make sure all high priority work is started before the low priority work is begun
-void priority_test(skiatest::Reporter* reporter, std::unique_ptr<SkExecutor> executor) {
- SkTaskGroup taskGroup(*executor);
- Collector collector;
-
- for (int i = 0; i < 100; ++i) {
- taskGroup.add([&collector, i]() {
- collector.insert(i);
- std::this_thread::sleep_for(std::chrono::microseconds(50));
- }, kHighPriority);
- }
- for (int i = 100; i < 200; ++i) {
- taskGroup.add([&collector, i]() {
- std::this_thread::sleep_for(std::chrono::milliseconds(1));
- collector.insert(i);
- }, kLowPriority);
- }
-
- taskGroup.wait();
-
- REPORTER_ASSERT(reporter, collector.count() == Collector::kMaxCount);
- REPORTER_ASSERT(reporter, collector.dataIsInOrder());
-}
-
-// Test out discarding
-void discard_test(skiatest::Reporter* reporter, std::unique_ptr<SkExecutor> executor) {
- SkTaskGroup taskGroup(*executor);
- Collector collector;
-
- for (int i = 0; i < 100; ++i) {
- executor->add([&collector, i]() {
- collector.insert(i);
- std::this_thread::sleep_for(std::chrono::microseconds(50));
- }, kHighPriority);
- }
- for (int i = 100; i < 200; ++i) {
- executor->add([&collector, i]() {
- std::this_thread::sleep_for(std::chrono::milliseconds(1));
- collector.insert(i);
- }, kLowPriority);
- }
- std::this_thread::sleep_for(std::chrono::milliseconds(3));
- executor->discardAllPendingWork();
-
- taskGroup.wait();
-
- // We should've shaved off some work
- REPORTER_ASSERT(reporter, collector.count() < Collector::kMaxCount);
- // But, the work that got done should still be in order
- REPORTER_ASSERT(reporter, collector.dataIsInOrder());
-}
-
-std::unique_ptr<SkExecutor> make_2x_FIFO() {
- return SkExecutor::MakeMultiListFIFOThreadPool(
- kNumWorkLists, kNumThreads, /* allowBorrowing= */ false);
-}
-
-std::unique_ptr<SkExecutor> make_2x_LIFO() {
- return SkExecutor::MakeMultiListLIFOThreadPool(
- kNumWorkLists, kNumThreads, /* allowBorrowing= */ false);
-}
-
-std::unique_ptr<SkExecutor> make_1x_FIFO() {
- return SkExecutor::MakeFIFOThreadPool(kNumThreads, /* allowBorrowing= */ false);
-}
-
-} // anonymous namespace
-
-DEF_TEST(Executor, reporter) {
- // 1x_LIFO is incompatible w/ how this test is structured
- for (auto makeExecutor : { make_2x_FIFO, make_2x_LIFO, make_1x_FIFO }) {
- priority_test(reporter, makeExecutor());
- discard_test(reporter, makeExecutor());
- }
-}
diff --git a/tests/testgroups.bzl b/tests/testgroups.bzl
index 2289f10..72d28bd 100644
--- a/tests/testgroups.bzl
+++ b/tests/testgroups.bzl
@@ -40,7 +40,6 @@
"DrawBitmapRectTest.cpp",
"DrawPathTest.cpp",
"EmptyPathTest.cpp",
- "ExecutorTest.cpp",
"F16StagesTest.cpp",
"FillPathTest.cpp",
"FitsInTest.cpp",
