From ee248599d49a15fc207c5aeb0b90ec263cc1d600 Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Thu, 12 Feb 2015 14:10:21 -0500 Subject: [PATCH] Refactor DisplayList path caching. This removes dependence on SkPath ptrs that HWUI does not control the lifecycle of. This clears up some errors where the paths are not generated from Java, but rather the Skia test suites. Cherry-pick of a change that originally landed in master-skia and is dependent on a skia merge (ag/655422). Change-Id: I41b9797a2b0af5d6b4ea51891565469d4f1d832d --- core/jni/android/graphics/Path.cpp | 10 ++--- libs/hwui/DisplayList.cpp | 15 +++++--- libs/hwui/DisplayList.h | 3 +- libs/hwui/DisplayListRenderer.h | 19 ++-------- libs/hwui/PathCache.cpp | 61 +++++++----------------------- libs/hwui/PathCache.h | 17 ++------- libs/hwui/ResourceCache.cpp | 44 --------------------- libs/hwui/ResourceCache.h | 7 ---- 8 files changed, 36 insertions(+), 140 deletions(-) diff --git a/core/jni/android/graphics/Path.cpp b/core/jni/android/graphics/Path.cpp index 831ce093c566b..f7b5dc24cea15 100644 --- a/core/jni/android/graphics/Path.cpp +++ b/core/jni/android/graphics/Path.cpp @@ -27,7 +27,7 @@ #include "SkPath.h" #include "SkPathOps.h" -#include +#include #include #include @@ -38,11 +38,11 @@ public: static void finalizer(JNIEnv* env, jobject clazz, jlong objHandle) { SkPath* obj = reinterpret_cast(objHandle); - if (android::uirenderer::ResourceCache::hasInstance()) { - android::uirenderer::ResourceCache::getInstance().destructor(obj); - } else { - delete obj; + // Purge entries from the HWUI path cache if this path's data is unique + if (obj->unique() && android::uirenderer::Caches::hasInstance()) { + android::uirenderer::Caches::getInstance().pathCache.removeDeferred(obj); } + delete obj; } static jlong init1(JNIEnv* env, jobject clazz) { diff --git a/libs/hwui/DisplayList.cpp b/libs/hwui/DisplayList.cpp index 317e395a852a6..4540bec77a9e4 100644 --- a/libs/hwui/DisplayList.cpp +++ b/libs/hwui/DisplayList.cpp @@ -49,18 +49,21 @@ void DisplayListData::cleanupResources() { resourceCache.decrementRefcountLocked(patchResources.itemAt(i)); } - for (size_t i = 0; i < sourcePaths.size(); i++) { - resourceCache.decrementRefcountLocked(sourcePaths.itemAt(i)); - } - resourceCache.unlock(); + for (size_t i = 0; i < pathResources.size(); i++) { + const SkPath* path = pathResources.itemAt(i); + if (path->unique() && Caches::hasInstance()) { + Caches::getInstance().pathCache.removeDeferred(path); + } + delete path; + } + bitmapResources.clear(); patchResources.clear(); - sourcePaths.clear(); + pathResources.clear(); paints.clear(); regions.clear(); - paths.clear(); } size_t DisplayListData::addChild(DrawRenderNodeOp* op) { diff --git a/libs/hwui/DisplayList.h b/libs/hwui/DisplayList.h index 011b5096c695b..3178584dea5a5 100644 --- a/libs/hwui/DisplayList.h +++ b/libs/hwui/DisplayList.h @@ -134,12 +134,11 @@ public: int projectionReceiveIndex; Vector bitmapResources; + Vector pathResources; Vector patchResources; std::vector> paints; std::vector> regions; - std::vector> paths; - SortedVector sourcePaths; Vector functors; const Vector& getChunks() const { diff --git a/libs/hwui/DisplayListRenderer.h b/libs/hwui/DisplayListRenderer.h index d313c18077f89..48ecd698bd916 100644 --- a/libs/hwui/DisplayListRenderer.h +++ b/libs/hwui/DisplayListRenderer.h @@ -282,21 +282,10 @@ private: inline const SkPath* refPath(const SkPath* path) { if (!path) return nullptr; - const SkPath* cachedPath = mPathMap.valueFor(path); - if (cachedPath == nullptr || cachedPath->getGenerationID() != path->getGenerationID()) { - SkPath* newPathCopy = new SkPath(*path); - newPathCopy->setSourcePath(path); - cachedPath = newPathCopy; - std::unique_ptr copy(newPathCopy); - mDisplayListData->paths.push_back(std::move(copy)); - - // replaceValueFor() performs an add if the entry doesn't exist - mPathMap.replaceValueFor(path, cachedPath); - } - if (mDisplayListData->sourcePaths.indexOf(path) < 0) { - mResourceCache.incrementRefcount(path); - mDisplayListData->sourcePaths.add(path); - } + // The points/verbs within the path are refcounted so this copy operation + // is inexpensive and maintains the generationID of the original path. + const SkPath* cachedPath = new SkPath(*path); + mDisplayListData->pathResources.add(cachedPath); return cachedPath; } diff --git a/libs/hwui/PathCache.cpp b/libs/hwui/PathCache.cpp index e2caf8bb45f93..5b2e5e2b8e1eb 100644 --- a/libs/hwui/PathCache.cpp +++ b/libs/hwui/PathCache.cpp @@ -362,22 +362,9 @@ void PathCache::PathProcessor::onProcess(const sp >& task) { // Paths /////////////////////////////////////////////////////////////////////////////// -void PathCache::remove(Vector& pathsToRemove, const path_pair_t& pair) { - LruCache::Iterator i(mCache); - - while (i.next()) { - const PathDescription& key = i.key(); - if (key.type == kShapePath && - (key.shape.path.mPath == pair.getFirst() || - key.shape.path.mPath == pair.getSecond())) { - pathsToRemove.push(key); - } - } -} - -void PathCache::removeDeferred(SkPath* path) { +void PathCache::removeDeferred(const SkPath* path) { Mutex::Autolock l(mLock); - mGarbage.push(path_pair_t(path, const_cast(path->getSourcePath()))); + mGarbage.push(path->getGenerationID()); } void PathCache::clearGarbage() { @@ -387,9 +374,15 @@ void PathCache::clearGarbage() { Mutex::Autolock l(mLock); size_t count = mGarbage.size(); for (size_t i = 0; i < count; i++) { - const path_pair_t& pair = mGarbage.itemAt(i); - remove(pathsToRemove, pair); - delete pair.getFirst(); + const uint32_t generationID = mGarbage.itemAt(i); + + LruCache::Iterator iter(mCache); + while (iter.next()) { + const PathDescription& key = iter.key(); + if (key.type == kShapePath && key.shape.path.mGenerationID == generationID) { + pathsToRemove.push(key); + } + } } mGarbage.clear(); } @@ -399,27 +392,9 @@ void PathCache::clearGarbage() { } } -/** - * To properly handle path mutations at draw time we always make a copy - * of paths objects when recording display lists. The source path points - * to the path we originally copied the path from. This ensures we use - * the original path as a cache key the first time a path is inserted - * in the cache. The source path is also used to reclaim garbage when a - * Dalvik Path object is collected. - */ -static const SkPath* getSourcePath(const SkPath* path) { - const SkPath* sourcePath = path->getSourcePath(); - if (sourcePath && sourcePath->getGenerationID() == path->getGenerationID()) { - return const_cast(sourcePath); - } - return path; -} - PathTexture* PathCache::get(const SkPath* path, const SkPaint* paint) { - path = getSourcePath(path); - PathDescription entry(kShapePath, paint); - entry.shape.path.mPath = path; + entry.shape.path.mGenerationID = path->getGenerationID(); PathTexture* texture = mCache.get(entry); @@ -442,11 +417,6 @@ PathTexture* PathCache::get(const SkPath* path, const SkPaint* paint) { texture = nullptr; mCache.remove(entry); } - } else if (path->getGenerationID() != texture->generation) { - // The size of the path might have changed so we first - // remove the entry from the cache - mCache.remove(entry); - texture = addTexture(entry, path, paint); } } @@ -458,19 +428,14 @@ void PathCache::precache(const SkPath* path, const SkPaint* paint) { return; } - path = getSourcePath(path); - PathDescription entry(kShapePath, paint); - entry.shape.path.mPath = path; + entry.shape.path.mGenerationID = path->getGenerationID(); PathTexture* texture = mCache.get(entry); bool generate = false; if (!texture) { generate = true; - } else if (path->getGenerationID() != texture->generation) { - mCache.remove(entry); - generate = true; } if (generate) { diff --git a/libs/hwui/PathCache.h b/libs/hwui/PathCache.h index 7378018b61ea8..23e35cb6b7173 100644 --- a/libs/hwui/PathCache.h +++ b/libs/hwui/PathCache.h @@ -118,7 +118,7 @@ struct PathDescription { SkPathEffect* pathEffect; union Shape { struct Path { - const SkPath* mPath; + uint32_t mGenerationID; } path; struct RoundRect { float mWidth; @@ -198,7 +198,7 @@ public: * Removes the specified path. This is meant to be called from threads * that are not the EGL context thread. */ - void removeDeferred(SkPath* path); + ANDROID_API void removeDeferred(const SkPath* path); /** * Process deferred removals. */ @@ -227,8 +227,6 @@ public: float& left, float& top, float& offset, uint32_t& width, uint32_t& height); private: - typedef Pair path_pair_t; - PathTexture* addTexture(const PathDescription& entry, const SkPath *path, const SkPaint* paint); PathTexture* addTexture(const PathDescription& entry, SkBitmap* bitmap); @@ -244,12 +242,6 @@ private: return mCache.get(entry); } - /** - * Removes an entry. - * The pair must define first=path, second=sourcePath - */ - void remove(Vector& pathsToRemove, const path_pair_t& pair); - /** * Ensures there is enough space in the cache for a texture of the specified * dimensions. @@ -279,8 +271,7 @@ private: delete future()->get(); } - // copied, since input path not refcounted / guaranteed to survive for duration of task - // TODO: avoid deep copy with refcounting + // copied, since input path not guaranteed to survive for duration of task const SkPath path; // copied, since input paint may not be immutable @@ -308,7 +299,7 @@ private: sp mProcessor; - Vector mGarbage; + Vector mGarbage; mutable Mutex mLock; }; // class PathCache diff --git a/libs/hwui/ResourceCache.cpp b/libs/hwui/ResourceCache.cpp index 2d1adc51679ac..d3b8d706e6063 100644 --- a/libs/hwui/ResourceCache.cpp +++ b/libs/hwui/ResourceCache.cpp @@ -79,10 +79,6 @@ void ResourceCache::incrementRefcount(void* resource, ResourceType resourceType) incrementRefcountLocked(resource, resourceType); } -void ResourceCache::incrementRefcount(const SkPath* pathResource) { - incrementRefcount((void*) pathResource, kPath); -} - void ResourceCache::incrementRefcount(const Res_png_9patch* patchResource) { incrementRefcount((void*) patchResource, kNinePatch); } @@ -107,10 +103,6 @@ void ResourceCache::decrementRefcount(const SkBitmap* bitmapResource) { decrementRefcountLocked(bitmapResource); } -void ResourceCache::decrementRefcount(const SkPath* pathResource) { - decrementRefcount((void*) pathResource); -} - void ResourceCache::decrementRefcount(const Res_png_9patch* patchResource) { decrementRefcount((void*) patchResource); } @@ -145,37 +137,10 @@ void ResourceCache::decrementRefcountLocked(const SkBitmap* bitmapResource) { } } -void ResourceCache::decrementRefcountLocked(const SkPath* pathResource) { - decrementRefcountLocked((void*) pathResource); -} - void ResourceCache::decrementRefcountLocked(const Res_png_9patch* patchResource) { decrementRefcountLocked((void*) patchResource); } -void ResourceCache::destructor(SkPath* resource) { - Mutex::Autolock _l(mLock); - destructorLocked(resource); -} - -void ResourceCache::destructorLocked(SkPath* resource) { - ssize_t index = mCache->indexOfKey(resource); - ResourceReference* ref = index >= 0 ? mCache->valueAt(index) : nullptr; - if (ref == nullptr) { - // If we're not tracking this resource, just delete it - if (Caches::hasInstance()) { - Caches::getInstance().pathCache.removeDeferred(resource); - } else { - delete resource; - } - return; - } - ref->destroyed = true; - if (ref->refCount == 0) { - deleteResourceReferenceLocked(resource, ref); - } -} - void ResourceCache::destructor(Res_png_9patch* resource) { Mutex::Autolock _l(mLock); destructorLocked(resource); @@ -208,15 +173,6 @@ void ResourceCache::destructorLocked(Res_png_9patch* resource) { void ResourceCache::deleteResourceReferenceLocked(const void* resource, ResourceReference* ref) { if (ref->destroyed) { switch (ref->resourceType) { - case kPath: { - SkPath* path = (SkPath*) resource; - if (Caches::hasInstance()) { - Caches::getInstance().pathCache.removeDeferred(path); - } else { - delete path; - } - } - break; case kNinePatch: { if (Caches::hasInstance()) { Caches::getInstance().patchCache.removeDeferred((Res_png_9patch*) resource); diff --git a/libs/hwui/ResourceCache.h b/libs/hwui/ResourceCache.h index 4333792afe42a..fae55d14fead5 100644 --- a/libs/hwui/ResourceCache.h +++ b/libs/hwui/ResourceCache.h @@ -20,7 +20,6 @@ #include #include -#include #include #include @@ -38,7 +37,6 @@ class Layer; */ enum ResourceType { kNinePatch, - kPath }; class ResourceReference { @@ -105,21 +103,16 @@ public: */ const SkBitmap* insert(const SkBitmap* resource); - void incrementRefcount(const SkPath* resource); void incrementRefcount(const Res_png_9patch* resource); void decrementRefcount(const SkBitmap* resource); - void decrementRefcount(const SkPath* resource); void decrementRefcount(const Res_png_9patch* resource); void decrementRefcountLocked(const SkBitmap* resource); - void decrementRefcountLocked(const SkPath* resource); void decrementRefcountLocked(const Res_png_9patch* resource); - void destructor(SkPath* resource); void destructor(Res_png_9patch* resource); - void destructorLocked(SkPath* resource); void destructorLocked(Res_png_9patch* resource); private: