AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Revert "redo AtlasTextOp caching"

Browse Source 
This reverts commit 3530d4d3c3.

Reason for revert: Use after free: context deleted before ops

Original change's description:
> redo AtlasTextOp caching
>
> The fuzzer ash_unittests is passing GrRecordingContext from thread
> to thread. This means that 120 bytes for a AtlasTextOp bytes are
> leaking on the first thread because the ClearCache is never
> called on that thread.
>
> Move the cache to the GrRecordingContext. Use a thread local to
> store a pointer to the GrRecordingContext so that new and delete can
> find the cache.
>
> Add a field to AtlasTextOp to save the recording context so that it can
> populate the thread local just before delete is called.
>
> Bug: chromium:1265033
>
> Change-Id: I9802910428bf091c534c96d4ce729c3b3445c76b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/497147
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Herb Derby <herb@google.com>

Bug: chromium:1265033
Change-Id: Ia51ad196cf2966225b177f799d477e1f705187c8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/498616
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
This commit is contained in:
Herb Derby 2022-01-24 15:44:29 +00:00 committed by SkCQ
parent 3530d4d3c3
commit 2a24e6554b
7 changed files with 101 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
include/gpu/GrRecordingContext.h
View File

@ -255,9 +255,6 @@ protected:
private:
OwnedArenas fArenas;
// Cache a single AtlasTextOp's worth of memory to improve op merging performance.
void* fAtlasTextOpCache = nullptr;
std::unique_ptr<GrDrawingManager> fDrawingManager;
std::unique_ptr<GrProxyProvider> fProxyProvider;

4
src/gpu/GrRecordingContext.cpp
View File

@ -48,7 +48,9 @@ GrRecordingContext::GrRecordingContext(sk_sp<GrContextThreadSafeProxy> proxy, bo
}
GrRecordingContext::~GrRecordingContext() {
::operator delete(fAtlasTextOpCache);
#if SK_GPU_V1
skgpu::v1::AtlasTextOp::ClearCache();
#endif
}
int GrRecordingContext::maxSurfaceSampleCountForColorType(SkColorType colorType) const {

3
src/gpu/GrRecordingContextPriv.h
View File

@ -203,9 +203,6 @@ public:
GrSurfaceOrigin,
sk_sp<GrRefCntedCallback> releaseHelper);
void* newAtlasTextOpBytes(size_t size);
void deleteAtlasTextOpBytes(void*);
protected:
explicit GrRecordingContextPriv(GrRecordingContext* rContext) : GrImageContextPriv(rContext) {}
// Required until C++17 copy elision

62
src/gpu/ops/AtlasTextOp.cpp
View File

@ -30,57 +30,32 @@
#include <new>
#include <utility>
void* GrRecordingContextPriv::newAtlasTextOpBytes(size_t size) {
if (this->context()->fAtlasTextOpCache != nullptr) {
return std::exchange(this->context()->fAtlasTextOpCache, nullptr);
}
return ::operator new(size);
}
void GrRecordingContextPriv::deleteAtlasTextOpBytes(void* bytes) {
if (this->context()->fAtlasTextOpCache == nullptr) {
this->context()->fAtlasTextOpCache = bytes;
return;
}
::operator delete(bytes);
}
namespace skgpu::v1 {
// Thread local holding the recording context which has the AtlasTextOp memory cache.
thread_local GrRecordingContext* AtlasTextOp::tlsRecordingContext = nullptr;
AtlasTextOp::~AtlasTextOp() {
for (const Geometry* g = fHead; g != nullptr;) {
const Geometry* next = g->fNext;
g->~Geometry();
g = next;
}
tlsRecordingContext = fRecordingContext;
}
void* AtlasTextOp::operator new(size_t size) {
auto context = tlsRecordingContext;
if (context != nullptr) {
return context->priv().newAtlasTextOpBytes(size);
// If we have thread local, then cache memory for a single AtlasTextOp.
static thread_local void* gCache = nullptr;
void* AtlasTextOp::operator new(size_t s) {
if (gCache != nullptr) {
return std::exchange(gCache, nullptr);
}
return ::operator new(size);
return ::operator new(s);
}
void AtlasTextOp::operator delete(void* bytes) noexcept {
auto context = tlsRecordingContext;
if (context != nullptr) {
context->priv().deleteAtlasTextOpBytes(bytes);
if (gCache == nullptr) {
gCache = bytes;
return;
}
::operator delete(bytes);
}
AtlasTextOp::AtlasTextOp(GrRecordingContext* recordingContext,
MaskType maskType,
void AtlasTextOp::ClearCache() {
::operator delete(gCache);
gCache = nullptr;
}
AtlasTextOp::AtlasTextOp(MaskType maskType,
bool needsTransform,
int glyphCount,
SkRect deviceRect,
@ -96,15 +71,13 @@ AtlasTextOp::AtlasTextOp(GrRecordingContext* recordingContext,
, fHasPerspective(needsTransform && geo->fDrawMatrix.hasPerspective())
, fUseGammaCorrectDistanceTable(false)
, fHead{geo}
, fTail{&fHead->fNext}
, fRecordingContext{recordingContext} {
, fTail{&fHead->fNext} {
// We don't have tight bounds on the glyph paths in device space. For the purposes of bounds
// we treat this as a set of non-AA rects rendered with a texture.
this->setBounds(deviceRect, HasAABloat::kNo, IsHairline::kNo);
}
AtlasTextOp::AtlasTextOp(GrRecordingContext* recordingContext,
MaskType maskType,
AtlasTextOp::AtlasTextOp(MaskType maskType,
bool needsTransform,
int glyphCount,
SkRect deviceRect,
@ -124,8 +97,7 @@ AtlasTextOp::AtlasTextOp(GrRecordingContext* recordingContext,
, fUseGammaCorrectDistanceTable(useGammaCorrectDistanceTable)
, fLuminanceColor(luminanceColor)
, fHead{geo}
, fTail{&fHead->fNext}
, fRecordingContext{recordingContext} {
, fTail{&fHead->fNext} {
// We don't have tight bounds on the glyph paths in device space. For the purposes of bounds
// we treat this as a set of non-AA rects rendered with a texture.
this->setBounds(deviceRect, HasAABloat::kNo, IsHairline::kNo);

34
src/gpu/ops/AtlasTextOp.h
View File

@ -20,16 +20,17 @@ class AtlasTextOp final : public GrMeshDrawOp {
public:
DEFINE_OP_CLASS_ID
template <typename... Args>
static GrOp::Owner Make(GrRecordingContext* context, Args&&... args) {
tlsRecordingContext = context;
return Owner{new skgpu::v1::AtlasTextOp(context, std::forward<Args>(args)...)};
~AtlasTextOp() override {
for (const Geometry* g = fHead; g != nullptr;) {
const Geometry* next = g->fNext;
g->~Geometry();
g = next;
}
}
~AtlasTextOp() override;
void* operator new(size_t s);
void operator delete(void* b) noexcept;
static void ClearCache();
static const int kVerticesPerGlyph = GrAtlasSubRun::kVerticesPerGlyph;
static const int kIndicesPerGlyph = 6;
@ -119,11 +120,6 @@ public:
private:
friend class GrOp; // for ctor
// tlsRecordingContext communicates the recording context from the Make and the dtor to new
// and delete. This allows new and delete to use a cache of AtlasTextOp memory which is
// stored on the RecordingContext.
static thread_local GrRecordingContext* tlsRecordingContext;
struct FlushInfo {
sk_sp<const GrBuffer> fVertexBuffer;
sk_sp<const GrBuffer> fIndexBuffer;
@ -134,16 +130,14 @@ private:
int fNumDraws = 0;
};
AtlasTextOp(GrRecordingContext* recordingContext,
MaskType maskType,
AtlasTextOp(MaskType maskType,
bool needsTransform,
int glyphCount,
SkRect deviceRect,
Geometry* geo,
GrPaint&& paint);
AtlasTextOp(GrRecordingContext* recordingContext,
MaskType maskType,
AtlasTextOp(MaskType maskType,
bool needsTransform,
int glyphCount,
SkRect deviceRect,
@ -258,19 +252,9 @@ private:
Geometry* fHead{nullptr};
Geometry** fTail{&fHead};
GrRecordingContext* const fRecordingContext;
using INHERITED = GrMeshDrawOp;
};
} // namespace skgpu::v1
// MakeAtlasTextOp needs to be located in this file to use the definition of AtlasTextOp because
// including AtlasTextOp.h in GrOp.h produces an include cycle.
template<typename... Args>
GrOp::Owner GrOp::MakeAtlasTextOp(GrRecordingContext* context, Args&&... args) {
return skgpu::v1::AtlasTextOp::Make(context, std::forward<Args>(args)...);
}
#endif // AtlasTextOp_DEFINED

7
src/gpu/ops/GrOp.h
View File

@ -26,7 +26,6 @@ class GrDstProxyView;
class GrOpFlushState;
class GrOpsRenderPass;
class GrPaint;
namespace skgpu { namespace v1 {class AtlasTextOp; }}
/**
* GrOp is the base class for all Ganesh deferred GPU operations. To facilitate reordering and to
@ -74,15 +73,9 @@ public:
template<typename Op, typename... Args>
static Owner Make(GrRecordingContext* context, Args&&... args) {
static_assert(!std::is_same<Op, skgpu::v1::AtlasTextOp>::value,
"Use MakeAtlasTextOp for AtlasTextOp.");
// Use MakeAtlasTextOp to take advantage of caching for AtlasTextOp.
return Owner{new Op(std::forward<Args>(args)...)};
}
template<typename... Args>
static Owner MakeAtlasTextOp(GrRecordingContext* context, Args&&... args);
template<typename Op, typename... Args>
static Owner MakeWithProcessorSet(
GrRecordingContext* context, const SkPMColor4f& color,

144
src/gpu/text/GrTextBlob.cpp
View File

@ -723,13 +723,13 @@ DirectMaskSubRun::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
false,
this->glyphCount(),
subRunBounds,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
false,
this->glyphCount(),
subRunBounds,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -1019,13 +1019,13 @@ TransformedMaskSubRun::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -1332,16 +1332,16 @@ SDFTSubRun::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -1899,13 +1899,13 @@ DirectMaskSubRunNoCache::makeAtlasTextOp(const GrClip* clip,
drawingColor
};
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
false,
this->glyphCount(),
fGlyphDeviceBounds,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
false,
this->glyphCount(),
fGlyphDeviceBounds,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -2107,13 +2107,13 @@ TransformedMaskSubRunNoCache::makeAtlasTextOp(const GrClip* clip,
};
GrRecordingContext* rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -2353,16 +2353,16 @@ SDFTSubRunNoCache::makeAtlasTextOp(const GrClip* clip,
};
GrRecordingContext* rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -2806,13 +2806,13 @@ DirectMaskSubRunSlug::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
!integerTranslate,
this->glyphCount(),
subRunDeviceBounds,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
!integerTranslate,
this->glyphCount(),
subRunDeviceBounds,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -3236,16 +3236,16 @@ SDFTSubRunSlug::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
maskType,
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
SkPaintPriv::ComputeLuminanceColor(paint),
useGammaCorrectDistanceTable,
DFGPFlags,
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}
@ -3449,13 +3449,13 @@ TransformedMaskSubRunSlug::makeAtlasTextOp(const GrClip* clip,
sdc->arenaAlloc());
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::MakeAtlasTextOp(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
GrOp::Owner op = GrOp::Make<AtlasTextOp>(rContext,
op_mask_type(fMaskFormat),
true,
this->glyphCount(),
this->deviceRect(drawMatrix, drawOrigin),
geometry,
std::move(grPaint));
return {clip, std::move(op)};
}