Bug 1268569 - Don't bother removing files from cache2/doomed dir after shutdown. r=michal
authorHonza Bambas <honzab.moz@firemni.cz>
Fri, 20 May 2016 09:04:00 +0200
changeset 337780 7ce4920b0faff60e0731abfccbe4b486c668b9de
parent 337779 bece6d2a5793234b26afab6c1217ecffa3a2f1b2
child 337781 7340f8d06cf02e320ded13bfe526a634fc63c635
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs1268569
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1268569 - Don't bother removing files from cache2/doomed dir after shutdown. r=michal
netwerk/cache2/CacheFileIOManager.cpp
netwerk/cache2/CacheFileIOManager.h
--- a/netwerk/cache2/CacheFileIOManager.cpp
+++ b/netwerk/cache2/CacheFileIOManager.cpp
@@ -106,46 +106,50 @@ CacheFileHandle::Release()
 }
 
 NS_INTERFACE_MAP_BEGIN(CacheFileHandle)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END_THREADSAFE
 
 CacheFileHandle::CacheFileHandle(const SHA1Sum::Hash *aHash, bool aPriority, PinningStatus aPinning)
   : mHash(aHash)
+  , mIsDoomed(false)
+  , mClosed(false)
   , mPriority(aPriority)
-  , mClosed(false)
   , mSpecialFile(false)
   , mInvalid(false)
   , mFileExists(false)
-  , mPinning(aPinning)
+  , mLeakIt(false)
   , mDoomWhenFoundPinned(false)
   , mDoomWhenFoundNonPinned(false)
+  , mPinning(aPinning)
   , mFileSize(-1)
   , mFD(nullptr)
 {
   // If we initialize mDoomed in the initialization list, that initialization is
   // not guaranteeded to be atomic.  Whereas this assignment here is guaranteed
   // to be atomic.  TSan will see this (atomic) assignment and be satisfied
   // that cross-thread accesses to mIsDoomed are properly synchronized.
   mIsDoomed = false;
   LOG(("CacheFileHandle::CacheFileHandle() [this=%p, hash=%08x%08x%08x%08x%08x]"
        , this, LOGSHA1(aHash)));
 }
 
 CacheFileHandle::CacheFileHandle(const nsACString &aKey, bool aPriority, PinningStatus aPinning)
   : mHash(nullptr)
+  , mIsDoomed(false)
+  , mClosed(false)
   , mPriority(aPriority)
-  , mClosed(false)
   , mSpecialFile(true)
   , mInvalid(false)
   , mFileExists(false)
-  , mPinning(aPinning)
+  , mLeakIt(false)
   , mDoomWhenFoundPinned(false)
   , mDoomWhenFoundNonPinned(false)
+  , mPinning(aPinning)
   , mFileSize(-1)
   , mFD(nullptr)
   , mKey(aKey)
 {
   // See comment above about the initialization of mIsDoomed.
   mIsDoomed = false;
   LOG(("CacheFileHandle::CacheFileHandle() [this=%p, key=%s]", this,
        PromiseFlatCString(aKey).get()));
@@ -167,26 +171,29 @@ void
 CacheFileHandle::Log()
 {
   nsAutoCString leafName;
   if (mFile) {
     mFile->GetNativeLeafName(leafName);
   }
 
   if (mSpecialFile) {
-    LOG(("CacheFileHandle::Log() - special file [this=%p, isDoomed=%d, "
-         "priority=%d, closed=%d, invalid=%d, fileExists=%d, fileSize=%lld, "
-         "leafName=%s, key=%s]", this, int(mIsDoomed), mPriority, mClosed, mInvalid,
-         mFileExists, mFileSize, leafName.get(), mKey.get()));
+    LOG(("CacheFileHandle::Log() - special file [this=%p, "
+         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, leakit=%d, "
+         "pinning=%d, fileExists=%d, fileSize=%lld, leafName=%s, key=%s]",
+         this,
+         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid), bool(mLeakIt),
+         mPinning, bool(mFileExists), mFileSize, leafName.get(), mKey.get()));
   } else {
-    LOG(("CacheFileHandle::Log() - entry file [this=%p, hash=%08x%08x%08x%08x"
-         "%08x, isDoomed=%d, priority=%d, closed=%d, invalid=%d, fileExists=%d,"
-         " fileSize=%lld, leafName=%s, key=%s]", this, LOGSHA1(mHash),
-         int(mIsDoomed), mPriority, mClosed, mInvalid, mFileExists, mFileSize,
-         leafName.get(), mKey.get()));
+    LOG(("CacheFileHandle::Log() - entry file [this=%p, hash=%08x%08x%08x%08x%08x, "
+         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, leakit=%d, "
+         "pinning=%d, fileExists=%d, fileSize=%lld, leafName=%s, key=%s]",
+         this, LOGSHA1(mHash),
+         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid), bool(mLeakIt),
+         mPinning, bool(mFileExists), mFileSize, leafName.get(), mKey.get()));
   }
 }
 
 uint32_t
 CacheFileHandle::FileSizeInK() const
 {
   MOZ_ASSERT(mFileSize != -1);
   uint64_t size64 = mFileSize;
@@ -1208,17 +1215,17 @@ CacheFileIOManager::ShutdownInternal()
     h->Log();
 
     // Close file handle
     if (h->mFD) {
       ReleaseNSPRHandleInternal(h);
     }
 
     // Remove file if entry is doomed or invalid
-    if (h->mFileExists && (h->mIsDoomed || h->mInvalid)) {
+    if (h->mFileExists && !h->mLeakIt && (h->mIsDoomed || h->mInvalid)) {
       LOG(("CacheFileIOManager::ShutdownInternal() - Removing file from disk"));
       h->mFile->Remove(false);
     }
 
     if (!h->IsSpecialFile() && !h->mIsDoomed &&
         (h->mInvalid || !h->mFileExists)) {
       CacheIndex::RemoveEntry(h->Hash());
     }
@@ -1778,17 +1785,17 @@ CacheFileIOManager::CloseHandleInternal(
   MOZ_ASSERT(CacheFileIOManager::IsOnIOThreadOrCeased());
 
   // Close file handle
   if (aHandle->mFD) {
     ReleaseNSPRHandleInternal(aHandle);
   }
 
   // Delete the file if the entry was doomed or invalid
-  if (aHandle->mIsDoomed || aHandle->mInvalid) {
+  if ((aHandle->mIsDoomed || aHandle->mInvalid) && MOZ_LIKELY(!aHandle->mLeakIt)) {
     LOG(("CacheFileIOManager::CloseHandleInternal() - Removing file from "
          "disk"));
     aHandle->mFile->Remove(false);
   }
 
   if (!aHandle->IsSpecialFile() && !aHandle->mIsDoomed &&
       (aHandle->mInvalid || !aHandle->mFileExists)) {
     CacheIndex::RemoveEntry(aHandle->Hash());
@@ -2265,21 +2272,22 @@ CacheFileIOManager::ReleaseNSPRHandleInt
   // Leak other handles when past the shutdown time maximum lag.
   if (
 #ifndef DEBUG
       ((aHandle->mInvalid || aHandle->mIsDoomed) &&
       MOZ_UNLIKELY(CacheObserver::ShuttingDown())) ||
 #endif
       MOZ_UNLIKELY(!aIgnoreShutdownLag &&
                    CacheObserver::IsPastShutdownIOLag())) {
-    // Pretend this file has been validated (the metadata has been written)
-    // to prevent removal I/O on this apparently used file.  The entry will
-    // never be used, since it doesn't have correct metadata, thus we don't
-    // need to worry about removing it.
-    aHandle->mInvalid = false;
+    // Don't bother closing (and removing) this file.
+    // For mInvalid == true the entry will never be used, since it doesn't
+    // have correct metadata, thus we don't need to worry about removing it.
+    // For mIsDoomed == true the file is already in the doomed sub-dir and
+    // will be removed on next session start.
+    aHandle->mLeakIt = true;
     LOG(("  past the shutdown I/O lag, leaking file handle"));
   } else {
     PR_Close(aHandle->mFD);
   }
 
   aHandle->mFD = nullptr;
 
   return NS_OK;
--- a/netwerk/cache2/CacheFileIOManager.h
+++ b/netwerk/cache2/CacheFileIOManager.h
@@ -75,40 +75,43 @@ public:
 private:
   friend class CacheFileIOManager;
   friend class CacheFileHandles;
   friend class ReleaseNSPRHandleEvent;
 
   virtual ~CacheFileHandle();
 
   const SHA1Sum::Hash *mHash;
-  mozilla::Atomic<bool,ReleaseAcquire> mIsDoomed;
-  bool                 mPriority;
-  bool                 mClosed;
-  bool                 mSpecialFile;
-  bool                 mInvalid;
-  bool                 mFileExists; // This means that the file should exists,
-                                    // but it can be still deleted by OS/user
-                                    // and then a subsequent OpenNSPRFileDesc()
-                                    // will fail.
+  mozilla::Atomic<bool, ReleaseAcquire> mIsDoomed;
+  mozilla::Atomic<bool, ReleaseAcquire> mClosed;
+  bool const           mPriority : 1;
+  bool const           mSpecialFile : 1;
 
+  // These bit flags are all accessed only on the IO thread
+  bool                 mInvalid : 1;
+  bool                 mFileExists : 1; // This means that the file should exists,
+                                        // but it can be still deleted by OS/user
+                                        // and then a subsequent OpenNSPRFileDesc()
+                                        // will fail.
+  bool                 mLeakIt : 1; // Don't close the file descriptor of this handle.
+
+  // Both initially false.  Can be raised to true only when this handle is to be doomed
+  // during the period when the pinning status is unknown.  After the pinning status
+  // determination we check these flags and possibly doom.
+  // These flags are only accessed on the IO thread.
+  bool                 mDoomWhenFoundPinned : 1;
+  bool                 mDoomWhenFoundNonPinned : 1;
   // For existing files this is always pre-set to UNKNOWN.  The status is udpated accordingly
   // after the matadata has been parsed.
   // For new files the flag is set according to which storage kind is opening
   // the cache entry and remains so for the handle's lifetime.
   // The status can only change from UNKNOWN (if set so initially) to one of PINNED or NON_PINNED
   // and it stays unchanged afterwards.
   // This status is only accessed on the IO thread.
   PinningStatus        mPinning;
-  // Both initially false.  Can be raised to true only when this handle is to be doomed
-  // during the period when the pinning status is unknown.  After the pinning status
-  // determination we check these flags and possibly doom.
-  // These flags are only accessed on the IO thread.
-  bool                 mDoomWhenFoundPinned : 1;
-  bool                 mDoomWhenFoundNonPinned : 1;
 
   nsCOMPtr<nsIFile>    mFile;
   int64_t              mFileSize;
   PRFileDesc          *mFD;  // if null then the file doesn't exists on the disk
   nsCString            mKey;
 };
 
 class CacheFileHandles {