Merge branch 'review-brentb-main' into 'main'

Ptex improvements

See merge request look/ptex!12

Author: Brent Burley

CMakeLists.txt

@@ -33,7 +33,7 @@ enable_testing()
 # Setup platform-specific threading flags.
 find_package(Threads REQUIRED)
 
-find_package(ZLIB REQUIRED)
+find_package(libdeflate REQUIRED)
 
 if (NOT DEFINED PTEX_SHA)
     # Query git for current commit ID

src/ptex/CMakeLists.txt

@@ -23,7 +23,9 @@ if(PTEX_BUILD_STATIC_LIBS)
     PRIVATE
         ${CMAKE_CURRENT_SOURCE_DIR})
     target_link_libraries(Ptex_static
-        PUBLIC Threads::Threads ZLIB::ZLIB)
+      PUBLIC Threads::Threads
+      PRIVATE libdeflate::libdeflate_static
+    )
     install(TARGETS Ptex_static EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR})
 endif()
 
@@ -39,7 +41,9 @@ if(PTEX_BUILD_SHARED_LIBS)
             ${CMAKE_CURRENT_SOURCE_DIR})
     target_compile_definitions(Ptex_dynamic PRIVATE PTEX_EXPORTS)
     target_link_libraries(Ptex_dynamic
-        PUBLIC Threads::Threads ZLIB::ZLIB)
+      PUBLIC Threads::Threads
+      PRIVATE libdeflate::libdeflate_shared
+    )
     install(TARGETS Ptex_dynamic EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR})
 endif()
 

src/ptex/PtexCache.cpp

@@ -82,6 +82,10 @@ PTEX_NAMESPACE_BEGIN
 
 void PtexCachedReader::release()
 {
+    // Unref the texture, and log the texture as recently used if the ref count becomes 0.  Note: If
+    // the ref is negative after unref is called, it means the unref'd texture was not previously in
+    // use which is an application error. In this case, the texture was presumably already logged as
+    // recently used and we shouldn't log it again.
     if (0 == unref()) {
         _cache->logRecentlyUsed(this);
     }
@@ -123,7 +127,6 @@ PtexTexture* PtexReaderCache::get(const char* filename, Ptex::String& error)
     // lookup reader in map
     StringKey key(filename);
     PtexCachedReader* reader = _files.get(key);
-    bool isNew = false;
 
     if (reader) {
         if (!reader->ok()) return 0;
@@ -134,43 +137,36 @@ PtexTexture* PtexReaderCache::get(const char* filename, Ptex::String& error)
         reader->ref();
     } else {
         reader = new PtexCachedReader(_premultiply, _io, _err, this);
-        isNew = true;
+        size_t newMemUsed = 0;
+        PtexCachedReader* newreader = reader;
+        reader = _files.tryInsert(key, reader, newMemUsed);
+        adjustMemUsed(newMemUsed);
+        if (reader != newreader) {
+            // another thread got here first
+            reader->ref();
+            delete newreader;
+        }
     }
 
-    bool needOpen = reader->needToOpen();
-    if (needOpen) {
+    if (reader->needToOpen()) {
         std::string buffer;
         const char* pathToOpen = filename;
         // search for the file (unless we have an I/O handler)
         if (_io || findFile(pathToOpen, buffer, error)) {
-            reader->open(pathToOpen, error);
+            if (reader->open(pathToOpen, error)) {
+                reader->logOpen();
+            }
         } else {
             // flag reader as invalid so we don't try to open it again on next lookup
             reader->invalidate();
         }
     }
 
-    if (isNew) {
-        size_t newMemUsed = 0;
-        PtexCachedReader* newreader = reader;
-        reader = _files.tryInsert(key, reader, newMemUsed);
-        adjustMemUsed(newMemUsed);
-        if (reader != newreader) {
-            // another thread got here first
-            reader->ref();
-            delete newreader;
-        }
-    }
-
     if (!reader->ok()) {
         reader->unref();
         return 0;
     }
 
-    if (needOpen) {
-        reader->logOpen();
-    }
-
     return reader;
 }
 
@@ -190,13 +186,14 @@ void PtexReaderCache::logRecentlyUsed(PtexCachedReader* reader)
     while (1) {
         MruList* mruList = _mruList;
         int slot = AtomicIncrement(&mruList->next)-1;
-        if (slot < numMruFiles) {
+        if (slot < maxMruFiles) {
             mruList->files[slot] = reader;
+            processMru();
             return;
         }
         // no mru slot available, process mru list and try again
         do processMru();
-        while (_mruList->next >= numMruFiles);
+        while (_mruList->next >= maxMruFiles);
     }
 }
 
@@ -205,7 +202,9 @@ void PtexReaderCache::processMru()
     // use a non-blocking lock so we can proceed as soon as space has been freed in the mru list
     // (which happens almost immediately in the processMru thread that has the lock)
     if (!_mruLock.trylock()) return;
-    if (_mruList->next < numMruFiles) {
+
+    if (!_mruList->next) {
+        // nothing to do
         _mruLock.unlock();
         return;
     }
@@ -217,7 +216,8 @@ void PtexReaderCache::processMru()
 
     // extract relevant stats and add to open/active list
     size_t memUsedChange = 0, filesOpenChange = 0;
-    for (int i = 0; i < numMruFiles; ++i) {
+    int numFilesToProcess = std::min(int(mruList->next), maxMruFiles);
+    for (int i = 0; i < numFilesToProcess; ++i) {
         PtexCachedReader* reader;
         do { reader = mruList->files[i]; } while (!reader); // loop on (unlikely) race condition
         mruList->files[i] = 0;
@@ -237,50 +237,55 @@ void PtexReaderCache::processMru()
     AtomicStore(&mruList->next, 0);
     adjustMemUsed(memUsedChange);
     adjustFilesOpen(filesOpenChange);
+    _mruLock.unlock();
 
-    bool shouldPruneFiles = _filesOpen > _maxFiles;
-    bool shouldPruneData = _maxMem && _memUsed > _maxMem;
-
-    if (shouldPruneFiles) {
-        pruneFiles();
-    }
-    if (shouldPruneData) {
-        pruneData();
+    pruneFilesIfNeeded();
+    if (_maxMem) {
+        pruneDataIfNeeded();
     }
-    _mruLock.unlock();
 }
 
 
-void PtexReaderCache::pruneFiles()
+void PtexReaderCache::pruneFilesIfNeeded()
 {
-    size_t numToClose = _filesOpen - _maxFiles;
-    if (numToClose > 0) {
-        while (numToClose) {
-            PtexCachedReader* reader = _openFiles.pop();
-            if (!reader) { _filesOpen = 0; break; }
-            if (reader->tryClose()) {
-                --numToClose;
-                --_filesOpen;
-            }
+    while (_filesOpen > _maxFiles) {
+        // close one file not currently in use, and then check again
+        _mruLock.lock();
+        PtexCachedReader* reader = _openFiles.pop();
+        _mruLock.unlock();
+
+        if (!reader) {
+            // no inactive open file available to close
+            break;
+        }
+        if (reader->tryClose()) {
+            --_filesOpen;
         }
     }
 }
 
 
-void PtexReaderCache::pruneData()
+void PtexReaderCache::pruneDataIfNeeded()
 {
     size_t memUsedChangeTotal = 0;
-    size_t memUsed = _memUsed;
-    while (memUsed + memUsedChangeTotal > _maxMem) {
+    while (_memUsed > _maxMem) {
+        // prune one texture not currently in use, and then check again
+        _mruLock.lock();
         PtexCachedReader* reader = _activeFiles.pop();
-        if (!reader) break;
+        _mruLock.unlock();
+        if (!reader) {
+            // no inactive texture available to prune
+            break;
+        }
         size_t memUsedChange;
         if (reader->tryPrune(memUsedChange)) {
             // Note: after clearing, memUsedChange is negative
             memUsedChangeTotal += memUsedChange;
         }
     }
-    adjustMemUsed(memUsedChangeTotal);
+    if (memUsedChangeTotal) {
+        adjustMemUsed(memUsedChangeTotal);
+    }
 }
 
 

src/ptex/PtexCache.h

@@ -136,15 +136,28 @@ class PtexCachedReader : public PtexReader
     ~PtexCachedReader() {}
 
     void ref() {
+        // Note: a negative ref count indicates the texture is locked, e.g. for pruning or purging.
+        // This should happen rarely, and only for a brief instant, but when it does we must wait
+        // for the ref count to become non-negative again. We do so by making CompareAndSwap fail
+        // when the old value is negative.
         while (1) {
-            int32_t oldCount = _refCount;
-            if (oldCount >= 0 && AtomicCompareAndSwap(&_refCount, oldCount, oldCount+1))
+            int32_t oldCount = std::max(0, int32_t(_refCount));
+            int32_t newCount = oldCount+1;
+            if (AtomicCompareAndSwap(&_refCount, oldCount, newCount))
                 return;
         }
     }
 
     int32_t unref() {
-        return AtomicDecrement(&_refCount);
+        int32_t newCount = AtomicDecrement(&_refCount);
+        if (newCount < 0) {
+            // A negative ref count here indicates an application error. The negative ref count also
+            // means the texture is now inadvertently locked. Set an error state, unlock the
+            // texture.
+            setError("PtexTexture Error: unref() called with refCount <= 0");
+            unlock();
+        }
+        return newCount;
     }
 
     virtual void release();
@@ -273,8 +286,8 @@ class PtexReaderCache : public PtexCache
 
     bool findFile(const char*& filename, std::string& buffer, Ptex::String& error);
     void processMru();
-    void pruneFiles();
-    void pruneData();
+    void pruneFilesIfNeeded();
+    void pruneDataIfNeeded();
     size_t _maxFiles;
     size_t _maxMem;
     PtexInputHandler* _io;
@@ -288,10 +301,10 @@ class PtexReaderCache : public PtexCache
     volatile size_t _filesOpen; CACHE_LINE_PAD(_filesOpen,size_t);
     Mutex _mruLock; CACHE_LINE_PAD(_mruLock,Mutex);
 
-    static const int numMruFiles = 50;
+    static const int maxMruFiles = 50;
     struct MruList {
         volatile int next;
-        PtexCachedReader* volatile files[numMruFiles];
+        PtexCachedReader* volatile files[maxMruFiles];
     };
     MruList _mruLists[2];
     MruList* volatile _mruList;