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

@@ -27,7 +27,7 @@
 #include "SkPath.h"
 #include "SkPathOps.h"
 
-#include <ResourceCache.h>
+#include <Caches.h>
 #include <vector>
 #include <map>
 
@@ -38,11 +38,11 @@ public:
 
     static void finalizer(JNIEnv* env, jobject clazz, jlong objHandle) {
         SkPath* obj = reinterpret_cast<SkPath*>(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) {

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) {

libs/hwui/DisplayList.h

@@ -134,12 +134,11 @@ public:
     int projectionReceiveIndex;
 
     Vector<const SkBitmap*> bitmapResources;
+    Vector<const SkPath*> pathResources;
     Vector<const Res_png_9patch*> patchResources;
 
     std::vector<std::unique_ptr<const SkPaint>> paints;
     std::vector<std::unique_ptr<const SkRegion>> regions;
-    std::vector<std::unique_ptr<const SkPath>> paths;
-    SortedVector<const SkPath*> sourcePaths;
     Vector<Functor*> functors;
 
     const Vector<Chunk>& getChunks() const {

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<const SkPath> 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;
     }
 

libs/hwui/PathCache.cpp

@@ -362,22 +362,9 @@ void PathCache::PathProcessor::onProcess(const sp<Task<SkBitmap*> >& task) {
 // Paths
 ///////////////////////////////////////////////////////////////////////////////
 
-void PathCache::remove(Vector<PathDescription>& pathsToRemove, const path_pair_t& pair) {
-    LruCache<PathDescription, PathTexture*>::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<SkPath*>(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<PathDescription, PathTexture*>::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<SkPath*>(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) {

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<SkPath*, SkPath*> 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<PathDescription>& 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<PathProcessor> mProcessor;
 
-    Vector<path_pair_t> mGarbage;
+    Vector<uint32_t> mGarbage;
     mutable Mutex mLock;
 }; // class PathCache
 

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);

libs/hwui/ResourceCache.h

@@ -20,7 +20,6 @@
 #include <cutils/compiler.h>
 
 #include <SkBitmap.h>
-#include <SkPath.h>
 #include <SkPixelRef.h>
 
 #include <utils/KeyedVector.h>
@@ -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: