From 3d102a33901d6498976ca8c753dbb9532b03e766 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sun, 17 Apr 2016 11:42:33 -0400 Subject: [PATCH] Access BlockFile::mFileName without copying, with proper multithreading cautions --- src/BlockFile.cpp | 4 +-- src/BlockFile.h | 21 +++++++++++- src/DirManager.cpp | 48 +++++++++++++++++++-------- src/Sequence.cpp | 4 +-- src/blockfile/ODDecodeBlockFile.cpp | 8 ++--- src/blockfile/ODDecodeBlockFile.h | 2 +- src/blockfile/ODPCMAliasBlockFile.cpp | 8 ++--- src/blockfile/ODPCMAliasBlockFile.h | 2 +- 8 files changed, 65 insertions(+), 32 deletions(-) diff --git a/src/BlockFile.cpp b/src/BlockFile.cpp index fdc746a87..813e900b8 100644 --- a/src/BlockFile.cpp +++ b/src/BlockFile.cpp @@ -120,9 +120,9 @@ BlockFile::~BlockFile() /// but most BlockFiles have at least their summary data here. /// (some, i.e. SilentBlockFiles, do not correspond to a file on /// disk and have empty file names) -wxFileName BlockFile::GetFileName() const +auto BlockFile::GetFileName() const -> GetFileNameResult { - return mFileName; + return { mFileName }; } ///sets the file name the summary info will be saved in. threadsafe. diff --git a/src/BlockFile.h b/src/BlockFile.h index 4542f59eb..3fbaf53c2 100644 --- a/src/BlockFile.h +++ b/src/BlockFile.h @@ -24,6 +24,8 @@ #include "wxFileNameWrapper.h" +#include "ondemand/ODTaskThread.h" + class SummaryInfo { public: @@ -72,7 +74,24 @@ class PROFILE_DLL_API BlockFile /* not final, abstract */ { /// Gets the filename of the disk file associated with this BlockFile /// (can be empty -- some BlockFiles, like SilentBlockFile, correspond to /// no file on disk) - virtual wxFileName GetFileName() const; + /// Avoids copying wxFileName by returning a reference, but for some subclasses + /// of BlockFile, you must exclude other threads from changing the name so long + /// as you have only a reference. Thus, this wrapper object that guarantees release + /// of any lock when it goes out of scope. Call mLocker.reset() to unlock it sooner. + struct GetFileNameResult { + const wxFileName &name; + ODLocker mLocker; + + GetFileNameResult(const wxFileName &name_, ODLocker &&locker = ODLocker{}) + : name{ name_ }, mLocker{ std::move(locker) } {} + + GetFileNameResult(const GetFileNameResult&) PROHIBITED; + GetFileNameResult &operator= (const GetFileNameResult&) PROHIBITED; + + GetFileNameResult(GetFileNameResult &&that) + : name{ that.name }, mLocker{ std::move(that.mLocker) } {} + }; + virtual GetFileNameResult GetFileName() const; virtual void SetFileName(wxFileNameWrapper &&name); virtual sampleCount GetLength() const { return mLen; } diff --git a/src/DirManager.cpp b/src/DirManager.cpp index 5644ddf0a..86882713c 100644 --- a/src/DirManager.cpp +++ b/src/DirManager.cpp @@ -941,7 +941,8 @@ bool DirManager::ContainsBlockFile(const BlockFile *b) const { if (!b) return false; - BlockHash::const_iterator it = mBlockFileHash.find(b->GetFileName().GetName()); + auto result = b->GetFileName(); + BlockHash::const_iterator it = mBlockFileHash.find(result.name.GetName()); return it != mBlockFileHash.end() && it->second == b; } @@ -957,7 +958,8 @@ bool DirManager::ContainsBlockFile(const wxString &filepath) const // the BlockFile. BlockFile *DirManager::CopyBlockFile(BlockFile *b) { - const auto &fn = b->GetFileName(); + auto result = b->GetFileName(); + const auto &fn = result.name; if (!b->IsLocked()) { b->Ref(); @@ -996,6 +998,9 @@ BlockFile *DirManager::CopyBlockFile(BlockFile *b) return NULL; } + // Done with fn + result.mLocker.reset(); + b2 = b->Copy(std::move(newFile)); if (b2 == NULL) @@ -1090,7 +1095,7 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs) // return a reference to the existing object instead. // - wxString name = target->GetFileName().GetName(); + wxString name = target->GetFileName().name.GetName(); BlockFile *retrieved = mBlockFileHash[name]; if (retrieved) { // Lock it in order to DELETE it safely, i.e. without having @@ -1114,23 +1119,24 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs) bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy) { - const auto oldFileName = f->GetFileName(); + auto result = f->GetFileName(); + const auto &oldFileNameRef = result.name; // Check that this BlockFile corresponds to a file on disk //ANSWER-ME: Is this checking only for SilentBlockFiles, in which case // (!oldFileName.IsOk()) is a more correct check? - if (oldFileName.GetName().IsEmpty()) { + if (oldFileNameRef.GetName().IsEmpty()) { return true; } wxFileNameWrapper newFileName; - if (!this->AssignFile(newFileName, oldFileName.GetFullName(), false)) + if (!this->AssignFile(newFileName, oldFileNameRef.GetFullName(), false)) return false; - if (newFileName != oldFileName) { + if (newFileName != oldFileNameRef) { //check to see that summary exists before we copy. bool summaryExisted = f->IsSummaryAvailable(); - auto oldPath = oldFileName.GetFullPath(); + auto oldPath = oldFileNameRef.GetFullPath(); auto newPath = newFileName.GetFullPath(); if (summaryExisted) { auto success = copy @@ -1139,11 +1145,21 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy) if (!success) return false; } - f->SetFileName(std::move(newFileName)); - //there is a small chance that the summary has begun to be computed on a different thread with the - //original filename. we need to catch this case by waiting for it to finish and then copy. if (!summaryExisted && (f->IsSummaryAvailable() || f->IsSummaryBeingComputed())) { + + // We will need to remember the old file name, so copy it + wxFileName oldFileName{ oldFileNameRef }; + + // Now we can free any lock (and should, if as the comment below says, we need + // the other threads to progress) + result.mLocker.reset(); + + f->SetFileName(std::move(newFileName)); + + //there is a small chance that the summary has begun to be computed on a different thread with the + //original filename. we need to catch this case by waiting for it to finish and then copy. + //block to make sure OD files don't get written while we are changing file names. //(It is important that OD files set this lock while computing their summary files.) while(f->IsSummaryBeingComputed() && !f->IsSummaryAvailable()) @@ -1160,6 +1176,11 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy) return false; } } + else { + // Can free this now, and must, because of nonrecursive mutexes + result.mLocker.reset(); + f->SetFileName(std::move(newFileName)); + } } return true; @@ -1190,7 +1211,7 @@ int DirManager::GetRefCount(BlockFile * f) void DirManager::Deref(BlockFile * f) { - wxString theFileName = f->GetFileName().GetName(); + const wxString theFileName = f->GetFileName().name.GetName(); //printf("Deref(%d): %s\n", // f->mRefCount-1, @@ -1338,7 +1359,8 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName) { ab->ChangeAliasedFileName(wxFileNameWrapper{ renamedFileName }); ab->UnlockRead(); - wxPrintf(_("Changed block %s to new alias name\n"), b->GetFileName().GetFullName().c_str()); + wxPrintf(_("Changed block %s to new alias name\n"), + b->GetFileName().name.GetFullName().c_str()); } else if (!b->IsDataAvailable() && db->GetEncodedAudioFilename() == fName) { diff --git a/src/Sequence.cpp b/src/Sequence.cpp index 9b1ae8a58..98040737c 100644 --- a/src/Sequence.cpp +++ b/src/Sequence.cpp @@ -987,7 +987,7 @@ void Sequence::HandleXMLEndTag(const wxChar *tag) for (unsigned b = 0, nn = mBlock.size(); b < nn; b++) { SeqBlock &block = mBlock[b]; if (block.start != numSamples) { - wxString sFileAndExtension = block.f->GetFileName().GetFullName(); + wxString sFileAndExtension = block.f->GetFileName().name.GetFullName(); if (sFileAndExtension.IsEmpty()) sFileAndExtension = wxT("(replaced with silence)"); else @@ -1829,7 +1829,7 @@ void Sequence::DebugPrintf(wxString *dest) const seqBlock.f ? mDirManager->GetRefCount(seqBlock.f) : 0); if (seqBlock.f) - *dest += seqBlock.f->GetFileName().GetFullName(); + *dest += seqBlock.f->GetFileName().name.GetFullName(); else *dest += wxT(""); diff --git a/src/blockfile/ODDecodeBlockFile.cpp b/src/blockfile/ODDecodeBlockFile.cpp index 355091289..4626118ab 100644 --- a/src/blockfile/ODDecodeBlockFile.cpp +++ b/src/blockfile/ODDecodeBlockFile.cpp @@ -369,13 +369,9 @@ void ODDecodeBlockFile::SetFileName(wxFileNameWrapper &&name) } ///sets the file name the summary info will be saved in. threadsafe. -wxFileName ODDecodeBlockFile::GetFileName() const +auto ODDecodeBlockFile::GetFileName() const -> GetFileNameResult { - wxFileName name; - mFileNameMutex.Lock(); - name = mFileName; - mFileNameMutex.Unlock(); - return name; + return { mFileName, ODLocker{ &mFileNameMutex } }; } /// A thread-safe version of CalcSummary. BlockFile::CalcSummary diff --git a/src/blockfile/ODDecodeBlockFile.h b/src/blockfile/ODDecodeBlockFile.h index 1a348daf6..fa51d80f4 100644 --- a/src/blockfile/ODDecodeBlockFile.h +++ b/src/blockfile/ODDecodeBlockFile.h @@ -133,7 +133,7 @@ class ODDecodeBlockFile final : public SimpleBlockFile ///sets the file name the summary info will be saved in. threadsafe. void SetFileName(wxFileNameWrapper &&name) override; - wxFileName GetFileName() const override; + GetFileNameResult GetFileName() const override; /// Prevents a read on other threads of the encoded audio file. void LockRead() const override; diff --git a/src/blockfile/ODPCMAliasBlockFile.cpp b/src/blockfile/ODPCMAliasBlockFile.cpp index 4f74fb794..b548da2e1 100644 --- a/src/blockfile/ODPCMAliasBlockFile.cpp +++ b/src/blockfile/ODPCMAliasBlockFile.cpp @@ -381,13 +381,9 @@ void ODPCMAliasBlockFile::SetFileName(wxFileNameWrapper &&name) } ///sets the file name the summary info will be saved in. threadsafe. -wxFileName ODPCMAliasBlockFile::GetFileName() const +auto ODPCMAliasBlockFile::GetFileName() const -> GetFileNameResult { - wxFileName name; - mFileNameMutex.Lock(); - name = mFileName; - mFileNameMutex.Unlock(); - return name; + return { mFileName, ODLocker{ &mFileNameMutex } }; } /// Write the summary to disk, using the derived ReadData() to get the data diff --git a/src/blockfile/ODPCMAliasBlockFile.h b/src/blockfile/ODPCMAliasBlockFile.h index 5a3f9acc0..2f1c465cb 100644 --- a/src/blockfile/ODPCMAliasBlockFile.h +++ b/src/blockfile/ODPCMAliasBlockFile.h @@ -125,7 +125,7 @@ class ODPCMAliasBlockFile final : public PCMAliasBlockFile ///sets the file name the summary info will be saved in. threadsafe. void SetFileName(wxFileNameWrapper &&name) override; - wxFileName GetFileName() const override; + GetFileNameResult GetFileName() const override; //when the file closes, it locks the blockfiles, but only conditionally. // It calls this so we can check if it has been saved before.