Skip to content

Commit f15a598

Browse files
egdanielSkia Commit-Bot
authored andcommitted
Revert "Reland "Create a basic IRNode pooling system.""
This reverts commit 5b09e6a. Reason for revert: breaking g3 Original change's description: > Reland "Create a basic IRNode pooling system." > > This is a reland of e16eca9 > > Original change's description: > > Create a basic IRNode pooling system. > > > > Allocations are redirected by overriding `operator new` and `operator > > delete` on the IRNode class. This allows us to use our existing > > `unique_ptr` and `make_unique` calls as-is. The Pool class is simple; > > it holds a fixed number of nodes and recycles them as they are returned. > > > > A fixed pool size of 2000 nodes was chosen. That is large enough to hold > > the contents of `sksl_large` during compilation, but it can be > > overflowed by very large shaders, or if multiple programs are converted > > at the same time. Exhausting the pool is not a problem; if this happens, > > additional nodes will be allocated via the system allocator as usual. > > More elaborate schemes are possible but might not add a lot of value. > > > > Thread safety is accomplished by placing the pool in a `thread_local` > > static during a Program's creation and destruction; the pool is freed > > when the program is destroyed. One important consequence of this > > strategy is that a program must free every node that it allocated during > > its creation, or else the node will be leaked. In debug, leaking a node > > will be detected and causes a DEBUGFAIL. In release, the pool will be > > freed despite having a live node in it, and if that node is later freed, > > that pointer will be passed to the system `free` (which is likely to > > cause a crash). > > > > In this CL, iOS does not support pooling, since support for > > `thread_local` was only added on iOS 9. This is fixed in the followup > > CL, http://review.skia.org/328837, which uses pthread keys on iOS. > > > > Nanobench shows ~15% improvement: > > (last week) http://screen/5CNBhTaZApcDA8h > > (today) http://screen/8ti5Rymvf6LUs8i > > > > Change-Id: I559de73606ee1be54e5eae7f82129dc928a63e3c > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/326876 > > Commit-Queue: John Stiles <johnstiles@google.com> > > Reviewed-by: Ethan Nicholas <ethannicholas@google.com> > > Auto-Submit: John Stiles <johnstiles@google.com> > > Change-Id: I114971e8e7ac0fabaf26216ae8813eeeaad0d4a2 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329086 > Reviewed-by: Ethan Nicholas <ethannicholas@google.com> > Commit-Queue: John Stiles <johnstiles@google.com> TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com Change-Id: Ie77a23366f2ba52fcbb0a751d11ca2792790a30c No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329165 Reviewed-by: Greg Daniel <egdaniel@google.com> Commit-Queue: Greg Daniel <egdaniel@google.com>
1 parent f73091b commit f15a598

File tree

6 files changed

+13
-281
lines changed

6 files changed

+13
-281
lines changed

gn/sksl.gni

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ skia_sksl_sources = [
3838
"$_src/sksl/SkSLMemoryLayout.h",
3939
"$_src/sksl/SkSLParser.cpp",
4040
"$_src/sksl/SkSLParser.h",
41-
"$_src/sksl/SkSLPool.cpp",
42-
"$_src/sksl/SkSLPool.h",
4341
"$_src/sksl/SkSLPosition.h",
4442
"$_src/sksl/SkSLRehydrator.cpp",
4543
"$_src/sksl/SkSLRehydrator.h",

src/sksl/SkSLCompiler.cpp

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1552,33 +1552,24 @@ std::unique_ptr<Program> Compiler::convertProgram(
15521552

15531553
const ParsedModule& baseModule = this->moduleForProgramKind(kind);
15541554

1555-
// Enable node pooling while converting and optimizing the program for a performance boost.
1556-
// The Program will take ownership of the pool.
1557-
std::unique_ptr<Pool> pool = Pool::CreatePoolOnThread(2000);
15581555
IRGenerator::IRBundle ir =
15591556
fIRGenerator->convertProgram(kind, &settings, baseModule, /*isBuiltinCode=*/false,
15601557
textPtr->c_str(), textPtr->size(), externalValues);
1561-
auto program = std::make_unique<Program>(kind,
1562-
std::move(textPtr),
1563-
settings,
1564-
fContext,
1565-
std::move(ir.fElements),
1566-
std::move(ir.fModifiers),
1567-
std::move(ir.fSymbolTable),
1568-
std::move(pool),
1569-
ir.fInputs);
1570-
bool success = false;
1558+
auto result = std::make_unique<Program>(kind,
1559+
std::move(textPtr),
1560+
settings,
1561+
fContext,
1562+
std::move(ir.fElements),
1563+
std::move(ir.fModifiers),
1564+
std::move(ir.fSymbolTable),
1565+
ir.fInputs);
15711566
if (fErrorCount) {
1572-
// Do not return programs that failed to compile.
1573-
} else if (settings.fOptimize && !this->optimize(*program)) {
1574-
// Do not return programs that failed to optimize.
1575-
} else {
1576-
// We have a successful program!
1577-
success = true;
1567+
return nullptr;
15781568
}
1579-
1580-
program->fPool->detachFromThread();
1581-
return success ? std::move(program) : nullptr;
1569+
if (settings.fOptimize && !this->optimize(*result)) {
1570+
return nullptr;
1571+
}
1572+
return result;
15821573
}
15831574

15841575
bool Compiler::optimize(Program& program) {

src/sksl/SkSLPool.cpp

Lines changed: 0 additions & 175 deletions
This file was deleted.

src/sksl/SkSLPool.h

Lines changed: 0 additions & 51 deletions
This file was deleted.

src/sksl/ir/SkSLIRNode.h

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
#include "src/sksl/SkSLASTNode.h"
1313
#include "src/sksl/SkSLLexer.h"
1414
#include "src/sksl/SkSLModifiersPool.h"
15-
#include "src/sksl/SkSLPool.h"
1615
#include "src/sksl/SkSLString.h"
1716

1817
#include <algorithm>
@@ -65,20 +64,6 @@ class IRNode {
6564
// purposes
6665
int fOffset;
6766

68-
// Override operator new and delete to allow us to control allocation behavior.
69-
static void* operator new(const size_t size) {
70-
// TODO: once all IRNodes hold their data in fData, everything should come out of the pool,
71-
// and this check should become an assertion.
72-
if (size == sizeof(IRNode)) {
73-
return Pool::AllocIRNode();
74-
}
75-
return ::operator new(size);
76-
}
77-
78-
static void operator delete(void* ptr) {
79-
Pool::FreeIRNode(ptr);
80-
}
81-
8267
protected:
8368
struct BlockData {
8469
std::shared_ptr<SymbolTable> fSymbolTable;

src/sksl/ir/SkSLProgram.h

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
namespace SkSL {
3333

3434
class Context;
35-
class Pool;
3635

3736
/**
3837
* Represents a fully-digested program, ready for code generation.
@@ -166,30 +165,16 @@ struct Program {
166165
std::vector<std::unique_ptr<ProgramElement>> elements,
167166
std::unique_ptr<ModifiersPool> modifiers,
168167
std::shared_ptr<SymbolTable> symbols,
169-
std::unique_ptr<Pool> pool,
170168
Inputs inputs)
171169
: fKind(kind)
172170
, fSource(std::move(source))
173171
, fSettings(settings)
174172
, fContext(context)
175173
, fSymbols(symbols)
176-
, fPool(std::move(pool))
177174
, fInputs(inputs)
178175
, fElements(std::move(elements))
179176
, fModifiers(std::move(modifiers)) {}
180177

181-
~Program() {
182-
// Some or all of the program elements are in the pool. To free them safely, we must attach
183-
// the pool before destroying any program elements. (Otherwise, we may accidentally call
184-
// delete on a pooled node.)
185-
fPool->attachToThread();
186-
fElements.clear();
187-
fContext.reset();
188-
fSymbols.reset();
189-
fModifiers.reset();
190-
fPool->detachFromThread();
191-
}
192-
193178
const std::vector<std::unique_ptr<ProgramElement>>& elements() const { return fElements; }
194179

195180
Kind fKind;
@@ -199,7 +184,6 @@ struct Program {
199184
// it's important to keep fElements defined after (and thus destroyed before) fSymbols,
200185
// because destroying elements can modify reference counts in symbols
201186
std::shared_ptr<SymbolTable> fSymbols;
202-
std::unique_ptr<Pool> fPool;
203187
Inputs fInputs;
204188

205189
private:

0 commit comments

Comments
 (0)