Merge pull request #5232 from richardkchapman/roxie-package-cache

HPCC-10564 Roxie is not caching dali lookups effectively

Reviewed-by: Gavin Halliday <ghalliday@hpccsystems.com>

roxie/ccd/ccdfile.cpp

@@ -1598,7 +1598,7 @@ CRoxieFileCache * fileCache;
 class CResolvedFile : public CInterface, implements IResolvedFileCreator
 {
 protected:
-    const IRoxiePackage *cached;
+    IResolvedFileCache *cached;
     StringAttr lfn;
     StringAttr physicalName;
     Owned<IDistributedFile> dFile; // NULL on copies serialized to slaves. Note that this implies we keep a lock on dali file for the lifetime of this object.
@@ -2063,9 +2063,15 @@ public:
         addSubFile(fdesc.getClear(), NULL);
     }
 
-    virtual void setCache(const IRoxiePackage *cache)
+    virtual void setCache(IResolvedFileCache *cache)
     {
-        assertex (!cached);
+        if (cached)
+        {
+            if (cache==NULL)
+                cached->removeCache(this);
+            else
+                throwUnexpected();
+        }
         cached = cache;
     }
 

roxie/ccd/ccdfile.hpp

@@ -85,7 +85,7 @@ interface IDefRecordMeta;
 interface IFilePartMap;
 class TranslatorArray;
 interface IInMemoryIndexManager ;
-interface IRoxiePackage;
+interface IResolvedFileCache;
 
 interface IResolvedFile : extends ISimpleSuperFileEnquiry
 {
@@ -102,7 +102,7 @@ interface IResolvedFile : extends ISimpleSuperFileEnquiry
 
     virtual const char *queryPhysicalName() const = 0; // Returns NULL unless in local file mode.
     virtual const char *queryFileName() const = 0;
-    virtual void setCache(const IRoxiePackage *cache) = 0;
+    virtual void setCache(IResolvedFileCache *cache) = 0;
     virtual bool isAlive() const = 0;
     virtual const IPropertyTree *queryProperties() const = 0;
 

roxie/ccd/ccdstate.cpp

@@ -182,43 +182,42 @@ public:
  * </PackageMaps>
  */
 
-class CRoxiePackageNode : extends CPackageNode, implements IRoxiePackage
+class CResolvedFileCache : implements IResolvedFileCache
 {
-protected:
-    Owned<IRoxieDaliHelper> daliHelper;
-
-    mutable CriticalSection cacheLock;
-    mutable CopyMapStringToMyClass<IResolvedFile> fileCache;
-
-    virtual aindex_t getBaseCount() const = 0;
-    virtual const CRoxiePackageNode *getBaseNode(aindex_t pos) const = 0;
+    CriticalSection cacheLock;
+    CopyMapStringToMyClass<IResolvedFile> files;
 
-    virtual bool getSysFieldTranslationEnabled() const {return fieldTranslationEnabled;} //roxie configured value
+public:
+    // Retrieve number of files in cache
+    inline unsigned count() const
+    {
+        return files.count();
+    }
 
     // Add a filename and the corresponding IResolvedFile to the cache
-    void addCache(const char *filename, const IResolvedFile *file) const
+    virtual void addCache(const char *filename, const IResolvedFile *file)
     {
         CriticalBlock b(cacheLock);
         IResolvedFile *add = const_cast<IResolvedFile *>(file);
         add->setCache(this);
-        fileCache.setValue(filename, add);
+        files.setValue(filename, add);
     }
     // Remove an IResolvedFile from the cache
-    void removeCache(const IResolvedFile *file) const
+    virtual void removeCache(const IResolvedFile *file)
     {
         CriticalBlock b(cacheLock);
         // NOTE: it's theoretically possible for the final release to happen after a replacement has been inserted into hash table. 
         // So only remove from hash table if what we find there matches the item that is being deleted.
-        IResolvedFile *goer = fileCache.getValue(file->queryFileName());
+        IResolvedFile *goer = files.getValue(file->queryFileName());
         if (goer == file)
-            fileCache.remove(file->queryFileName());
+            files.remove(file->queryFileName());
         // You might want to remove files from the daliServer cache too, but it's not safe to do so here as there may be multiple package caches
     }
     // Lookup a filename in the cache
-    IResolvedFile *lookupCache(const char *filename) const
+    virtual IResolvedFile *lookupCache(const char *filename)
     {
         CriticalBlock b(cacheLock);
-        IResolvedFile *cache = fileCache.getValue(filename);
+        IResolvedFile *cache = files.getValue(filename);
         if (cache)
         {
             LINK(cache);
@@ -227,6 +226,18 @@ protected:
         }
         return NULL;
     }
+};
+
+class CRoxiePackageNode : extends CPackageNode, implements IRoxiePackage
+{
+protected:
+    static CResolvedFileCache daliFiles;
+    mutable CResolvedFileCache fileCache;
+
+    virtual aindex_t getBaseCount() const = 0;
+    virtual const CRoxiePackageNode *getBaseNode(aindex_t pos) const = 0;
+
+    virtual bool getSysFieldTranslationEnabled() const {return fieldTranslationEnabled;} //roxie configured value
 
     // Use local package file only to resolve subfile into physical file info
     IResolvedFile *resolveLFNusingPackage(const char *fileName) const
@@ -246,16 +257,20 @@ protected:
     }
 
     // Use dali to resolve subfile into physical file info
-    IResolvedFile *resolveLFNusingDali(const char *fileName, bool cacheIt, bool writeAccess, bool alwaysCreate) const
+    static IResolvedFile *resolveLFNusingDaliOrLocal(const char *fileName, bool cacheIt, bool writeAccess, bool alwaysCreate)
     {
         // MORE - look at alwaysCreate... This may be useful to implement earlier locking semantics.
+        IResolvedFile* result = daliFiles.lookupCache(fileName);
+        if (result)
+            return result;
+        Owned<IRoxieDaliHelper> daliHelper = connectToDali();
         if (daliHelper)
         {
             if (daliHelper->connected())
             {
                 Owned<IDistributedFile> dFile = daliHelper->resolveLFN(fileName, cacheIt, writeAccess);
                 if (dFile)
-                    return createResolvedFile(fileName, NULL, dFile.getClear(), daliHelper, cacheIt, writeAccess);
+                    result = createResolvedFile(fileName, NULL, dFile.getClear(), daliHelper, cacheIt, writeAccess);
             }
             else if (!writeAccess)  // If we need write access and expect a dali, but don't have one, we should probably fail
             {
@@ -263,19 +278,14 @@ protected:
                 Owned<IFileDescriptor> fd = daliHelper->resolveCachedLFN(fileName);
                 if (fd)
                 {
-                    Owned <IResolvedFileCreator> result = createResolvedFile(fileName, NULL, false);
+                    Owned <IResolvedFileCreator> creator = createResolvedFile(fileName, NULL, false);
                     Owned<IFileDescriptor> remoteFDesc = daliHelper->checkClonedFromRemote(fileName, fd, cacheIt);
-                    result->addSubFile(fd.getClear(), remoteFDesc.getClear());
-                    return result.getClear();
+                    creator->addSubFile(fd.getClear(), remoteFDesc.getClear());
+                    result = creator.getClear();
                 }
             }
         }
-        return NULL;
-    }
-    // Use local package file's localFile info to resolve subfile into physical file info
-    IResolvedFile *resolveLFNusingLocal(const char *fileName, bool writeAccess, bool alwaysCreate) const
-    {
-        if (node && node->getPropBool("@localFiles"))
+        if (!result)
         {
             StringBuffer useName;
             if (strstr(fileName,"::"))
@@ -288,24 +298,33 @@ protected:
             bool exists = checkFileExists(useName);
             if (exists || alwaysCreate)
             {
-                Owned <IResolvedFileCreator> result = createResolvedFile(fileName, useName, false);
+                Owned <IResolvedFileCreator> creator = createResolvedFile(fileName, useName, false);
                 if (exists)
-                    result->addSubFile(useName);
-                return result.getClear();
+                    creator->addSubFile(useName);
+                result = creator.getClear();
             }
         }
-        return NULL;
+        if (result && cacheIt)
+            daliFiles.addCache(fileName, result);
+        return result;
     }
+
     // Use local package and its bases to resolve existing file into physical file info via all supported resolvers
+    IResolvedFile *lookupExpandedFileName(const char *fileName, bool cache, bool writeAccess, bool alwaysCreate) const
+    {
+        IResolvedFile *result = lookupFile(fileName, cache, writeAccess, alwaysCreate);
+        if (!result)
+            result = resolveLFNusingDaliOrLocal(fileName, cache, writeAccess, alwaysCreate);
+        return result;
+    }
+
     IResolvedFile *lookupFile(const char *fileName, bool cache, bool writeAccess, bool alwaysCreate) const
     {
         // Order of resolution: 
         // 1. Files named in package
-        // 2. If dali lookup enabled, dali
-        // 3. If local file system lookup enabled, local file system?
-        // 4. Files named in bases
+        // 2. Files named in bases
 
-        IResolvedFile* result = lookupCache(fileName);
+        IResolvedFile* result = fileCache.lookupCache(fileName);
         if (result)
             return result;
 
@@ -320,7 +339,7 @@ protected:
             {
                 StringBuffer subFileName;
                 subFileInfo->getSubFileName(idx, subFileName);
-                Owned<const IResolvedFile> subFileInfo = lookupFile(subFileName, cache, writeAccess, alwaysCreate);
+                Owned<const IResolvedFile> subFileInfo = lookupExpandedFileName(subFileName, cache, false, false);  // NOTE - overwriting a superfile does NOT require write access to subfiles
                 if (subFileInfo)
                 {
                     if (!super)
@@ -329,18 +348,14 @@ protected:
                 }
             }
             if (super && cache)
-                addCache(fileName, super);
+                fileCache.addCache(fileName, super);
             return super.getClear();
         }
         result = resolveLFNusingPackage(fileName);
-        if (!result)
-            result = resolveLFNusingDali(fileName, cache, writeAccess, alwaysCreate);
-        if (!result)
-            result = resolveLFNusingLocal(fileName, writeAccess, alwaysCreate);
         if (result)
         {
             if (cache)
-                addCache(fileName, result);
+                fileCache.addCache(fileName, result);
             return result;
         }
         aindex_t count = getBaseCount();
@@ -366,7 +381,6 @@ public:
 
     CRoxiePackageNode(IPropertyTree *p) : CPackageNode(p)
     {
-        daliHelper.setown(connectToDali()); // MORE - should make this conditional?
         if (!fileNameServiceDali.length())
             node->setPropBool("@localFiles", true);
     }
@@ -392,7 +406,7 @@ public:
         if (traceLevel > 5)
             DBGLOG("lookupFileName %s", fileName.str());
 
-        const IResolvedFile *result = lookupFile(fileName, cache, false, false);
+        const IResolvedFile *result = lookupExpandedFileName(fileName, cache, false, false);
         if (!result)
         {
             if (!opt)
@@ -408,6 +422,8 @@ public:
         StringBuffer fileName;
         expandLogicalFilename(fileName, _fileName, wu, false);
         Owned<IResolvedFile> resolved = lookupFile(fileName, false, true, true);
+        if (!resolved)
+            resolved.setown(resolveLFNusingDaliOrLocal(fileName, false, true, true));
         if (resolved)
         {
             if (resolved->exists())
@@ -416,13 +432,13 @@ public:
                     throw MakeStringException(99, "Cannot write %s, file already exists (missing OVERWRITE attribute?)", resolved->queryFileName());
                 if (extend)
                     UNIMPLEMENTED; // How does extend fit in with the clusterwritemanager stuff? They can't specify cluster and extend together...
-                removeCache(resolved);
-                resolved->remove();
+                resolved->setCache(NULL);
             }
             if (resolved->queryPhysicalName())
                 fileName.clear().append(resolved->queryPhysicalName());
             resolved.clear();
         }
+        Owned<IRoxieDaliHelper> daliHelper = connectToDali();
         bool disconnected = !daliHelper->connected();
         // MORE - not sure this is really the right test. If there SHOULD be a dali but is's unavailable, we should fail.
         Owned<ILocalOrDistributedFile> ldFile = createLocalOrDistributedFile(fileName, NULL, disconnected, !disconnected, true);
@@ -459,6 +475,8 @@ public:
     }
 };
 
+CResolvedFileCache CRoxiePackageNode::daliFiles;
+
 typedef CResolvedPackage<CRoxiePackageNode> CRoxiePackage;
 
 IRoxiePackage *createRoxiePackage(IPropertyTree *p, IRoxiePackageMap *packages)

roxie/ccd/ccdstate.hpp

@@ -60,8 +60,16 @@ interface IRoxiePackage : public IHpccPackage
     virtual IRoxieWriteHandler *createFileName(const char *fileName, bool overwrite, bool extend, const StringArray &clusters, IConstWorkUnit *wu) const = 0;
     // Lookup information in package about what in-memory indexes should be built for file
     virtual IPropertyTreeIterator *getInMemoryIndexInfo(const IPropertyTree &graphNode) const = 0;
-    // Remove a resolved file from the cache 
-    virtual void removeCache(const IResolvedFile *goer) const = 0; // note that this is const as cache is considered mutable
+};
+
+interface IResolvedFileCache
+{
+    // Add a filename and the corresponding IResolvedFile to the cache
+    virtual void addCache(const char *filename, const IResolvedFile *file) = 0;
+    // Lookup a filename in the cache
+    virtual IResolvedFile *lookupCache(const char *filename) = 0;
+    // Remove a resolved file from the cache
+    virtual void removeCache(const IResolvedFile *goer) = 0;
 };
 
 extern IRoxiePackage *createRoxiePackage(IPropertyTree *p, IRoxiePackageMap *packages);