From 3c378b348c1f73fe816e847c54b0c12ff334f6b2 Mon Sep 17 00:00:00 2001 From: Scott Romero <24445312+AMZN-ScottR@users.noreply.github.com> Date: Mon, 19 Jul 2021 16:14:41 -0700 Subject: [PATCH] [development] Fixed runaway memory in editor (#2220) There was a change in bahaviour to how files/directories were collected in the AZ::IO::FindData::Scan* functions which allowed the addition of duplicate entries. This created a problem when attempting a recursively greedy search with zips included because of the level system pak files. Their mount points would be the root "levels" folder, so for N-levels there would be N identical entries to the "levels" folder being added perpetually each time ScanZips was called on the "levels" folder or above folders with "*" filtering. Letting the editor sit idle no longer sees the reported memory in the bottom status bar climb. Also, running the tests which prompted the initial change shows a negligible change in perf. Signed-off-by: AMZN-ScottR 24445312+AMZN-ScottR@users.noreply.github.com --- .../AzFramework/Archive/ArchiveFindData.cpp | 35 +++++++++++++------ .../AzFramework/Archive/ArchiveFindData.h | 14 ++++++-- .../Code/Tests/AudioSystemEditorTest.cpp | 4 +-- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.cpp b/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.cpp index 04e937905c..c4c845a832 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.cpp +++ b/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.cpp @@ -15,6 +15,11 @@ namespace AZ::IO { + size_t ArchiveFileIteratorHash::operator()(const AZ::IO::ArchiveFileIterator& iter) const + { + return iter.GetHash(); + } + bool AZStdStringLessCaseInsensitive::operator()(AZStd::string_view left, AZStd::string_view right) const { // If one or both strings are 0-length, return true if the left side is smaller, false if they're equal or left is larger. @@ -68,11 +73,21 @@ namespace AZ::IO return operator++(); } + bool ArchiveFileIterator::operator==(const AZ::IO::ArchiveFileIterator& rhs) const + { + return GetHash() == rhs.GetHash(); + } + ArchiveFileIterator::operator bool() const { return m_findData && m_lastFetchValid; } + size_t ArchiveFileIterator::GetHash() const + { + return AZStd::hash{}(m_filename.c_str()); + } + void FindData::Scan(IArchive* archive, AZStd::string_view szDir, bool bAllowUseFS, bool bScanZips) { // get the priority into local variable to avoid it changing in the course of @@ -113,14 +128,12 @@ namespace AZ::IO } AZ::IO::FileIOBase::GetDirectInstance()->FindFiles(searchDirectory.c_str(), pattern.c_str(), [&](const char* filePath) -> bool { - AZ::IO::ArchiveFileIterator fileIterator; - fileIterator.m_filename = AZ::IO::PathView(filePath).Filename().Native(); - fileIterator.m_fileDesc.nAttrib = {}; + AZ::IO::ArchiveFileIterator fileIterator{ nullptr, AZ::IO::PathView(filePath).Filename().Native(), {} }; if (AZ::IO::FileIOBase::GetDirectInstance()->IsDirectory(filePath)) { fileIterator.m_fileDesc.nAttrib = fileIterator.m_fileDesc.nAttrib | AZ::IO::FileDesc::Attribute::Subdirectory; - m_fileStack.emplace_back(AZStd::move(fileIterator)); + m_fileSet.emplace(AZStd::move(fileIterator)); } else { @@ -136,7 +149,7 @@ namespace AZ::IO // These times are not supported by our file interface fileIterator.m_fileDesc.tAccess = fileIterator.m_fileDesc.tWrite; fileIterator.m_fileDesc.tCreate = fileIterator.m_fileDesc.tWrite; - m_fileStack.emplace_back(AZStd::move(fileIterator)); + m_fileSet.emplace(AZStd::move(fileIterator)); } return true; }); @@ -167,7 +180,7 @@ namespace AZ::IO fileDesc.nAttrib = AZ::IO::FileDesc::Attribute::ReadOnly | AZ::IO::FileDesc::Attribute::Archive; fileDesc.nSize = fileEntry->desc.lSizeUncompressed; fileDesc.tWrite = fileEntry->GetModificationTime(); - m_fileStack.emplace_back(AZ::IO::ArchiveFileIterator{ this, fname, fileDesc }); + m_fileSet.emplace(AZ::IO::ArchiveFileIterator{ this, fname, fileDesc }); } ZipDir::FindDir findDirectoryEntry(zipCache); @@ -180,7 +193,7 @@ namespace AZ::IO } AZ::IO::FileDesc fileDesc; fileDesc.nAttrib = AZ::IO::FileDesc::Attribute::ReadOnly | AZ::IO::FileDesc::Attribute::Archive | AZ::IO::FileDesc::Attribute::Subdirectory; - m_fileStack.emplace_back(AZ::IO::ArchiveFileIterator{ this, fname, fileDesc }); + m_fileSet.emplace(AZ::IO::ArchiveFileIterator{ this, fname, fileDesc }); } }; @@ -249,7 +262,7 @@ namespace AZ::IO if (!bindRootIter->empty() && AZStd::wildcard_match(sourcePathRemainder.Native(), bindRootIter->Native())) { AZ::IO::FileDesc fileDesc{ AZ::IO::FileDesc::Attribute::ReadOnly | AZ::IO::FileDesc::Attribute::Archive | AZ::IO::FileDesc::Attribute::Subdirectory }; - m_fileStack.emplace_back(AZ::IO::ArchiveFileIterator{ this, bindRootIter->Native(), fileDesc }); + m_fileSet.emplace(AZ::IO::ArchiveFileIterator{ this, bindRootIter->Native(), fileDesc }); } } else @@ -265,7 +278,7 @@ namespace AZ::IO AZ::IO::ArchiveFileIterator FindData::Fetch() { - if (m_fileStack.empty()) + if (m_fileSet.empty()) { AZ::IO::ArchiveFileIterator emptyFileIterator; emptyFileIterator.m_lastFetchValid = false; @@ -274,10 +287,10 @@ namespace AZ::IO } // Remove Fetched item from the FindData map so that the iteration continues - AZ::IO::ArchiveFileIterator fileIterator{ m_fileStack.back() }; + AZ::IO::ArchiveFileIterator fileIterator{ *m_fileSet.begin() }; fileIterator.m_lastFetchValid = true; fileIterator.m_findData = this; - m_fileStack.pop_back(); + m_fileSet.erase(m_fileSet.begin()); return fileIterator; } } diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.h b/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.h index d2d7646676..12544d124d 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.h +++ b/Code/Framework/AzFramework/AzFramework/Archive/ArchiveFindData.h @@ -8,6 +8,7 @@ #pragma once +#include #include #include @@ -44,8 +45,12 @@ namespace AZ::IO ArchiveFileIterator operator++(); ArchiveFileIterator operator++(int); + bool operator==(const AZ::IO::ArchiveFileIterator& rhs) const; + explicit operator bool() const; + size_t GetHash() const; + inline static constexpr size_t FilenameMaxLength = 256; AZStd::fixed_string m_filename; FileDesc m_fileDesc; @@ -56,6 +61,11 @@ namespace AZ::IO bool m_lastFetchValid{}; }; + struct ArchiveFileIteratorHash + { + size_t operator()(const AZ::IO::ArchiveFileIterator& iter) const; + }; + struct AZStdStringLessCaseInsensitive { bool operator()(AZStd::string_view left, AZStd::string_view right) const; @@ -75,7 +85,7 @@ namespace AZ::IO void ScanFS(IArchive* archive, AZStd::string_view path); void ScanZips(IArchive* archive, AZStd::string_view path); - using FileStack = AZStd::vector; - FileStack m_fileStack; + using FileSet = AZStd::unordered_set; + FileSet m_fileSet; }; } diff --git a/Gems/AudioSystem/Code/Tests/AudioSystemEditorTest.cpp b/Gems/AudioSystem/Code/Tests/AudioSystemEditorTest.cpp index e8ab9b278e..a486e24326 100644 --- a/Gems/AudioSystem/Code/Tests/AudioSystemEditorTest.cpp +++ b/Gems/AudioSystem/Code/Tests/AudioSystemEditorTest.cpp @@ -38,7 +38,7 @@ namespace CustomMocks fileDesc.nSize = sizeof(AZ::IO::FileDesc); // Add a filename and file description reference to the TestFindData map to make sure the file iterator is valid m_findData = new TestFindData(); - m_findData->m_fileStack.emplace_back(AZ::IO::ArchiveFileIterator{ static_cast(m_findData.get()), m_levelName, fileDesc }); + m_findData->m_fileSet.emplace(AZ::IO::ArchiveFileIterator{ static_cast(m_findData.get()), m_levelName, fileDesc }); return m_findData->Fetch(); } @@ -54,7 +54,7 @@ namespace CustomMocks struct TestFindData : AZ::IO::FindData { - using AZ::IO::FindData::m_fileStack; + using AZ::IO::FindData::m_fileSet; }; AZStd::intrusive_ptr m_findData;