From 4609e6d285845073ec6d3e5036168fdf9b1103fb Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 13 Sep 2018 11:07:28 -0700 Subject: [PATCH 1/3] [Driver] Remove handrolled StringSetIterator in favor of 'keys()' An llvm::StringSet is "just" an llvm::StringMap where the values are ignored, so the 'keys()' range already does what we need. No functionality change. --- include/swift/Driver/DependencyGraph.h | 32 ++------------------------ 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/include/swift/Driver/DependencyGraph.h b/include/swift/Driver/DependencyGraph.h index b6db78798b081..73da043cb45d9 100644 --- a/include/swift/Driver/DependencyGraph.h +++ b/include/swift/Driver/DependencyGraph.h @@ -143,33 +143,6 @@ class DependencyGraphImpl { LoadResult loadFromBuffer(const void *node, llvm::MemoryBuffer &buffer); - // FIXME: We should be able to use llvm::mapped_iterator for this, but - // StringMapConstIterator isn't quite an InputIterator (no ->). - class StringSetIterator { - llvm::StringSet<>::const_iterator I; - public: - using value_type = llvm::StringSet<>::const_iterator::value_type; - using iterator_category = std::input_iterator_tag; - using difference_type = ptrdiff_t; - using reference = value_type &; - using pointer = value_type *; - - /*implicit*/ StringSetIterator(llvm::StringSet<>::const_iterator base) - : I(base) {} - - StringSetIterator &operator++() { - ++I; - return *this; - } - - StringRef operator*() const { - return I->getKey(); - } - - bool operator==(StringSetIterator other) const { return I == other.I; } - bool operator!=(StringSetIterator other) const { return I != other.I; } - }; - protected: LoadResult loadFromString(const void *node, StringRef data); LoadResult loadFromPath(const void *node, StringRef path); @@ -195,9 +168,8 @@ class DependencyGraphImpl { } public: - llvm::iterator_range getExternalDependencies() const { - return llvm::make_range(StringSetIterator(ExternalDependencies.begin()), - StringSetIterator(ExternalDependencies.end())); + decltype(ExternalDependencies.keys()) getExternalDependencies() const { + return ExternalDependencies.keys(); } }; From 245b06b8323114269c8651fb4d8e2ce7918d4c25 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 13 Sep 2018 11:09:01 -0700 Subject: [PATCH 2/3] Use llvm::StringSet instead of StringMap where appropriate That is, where we aren't using the value for anything. No functionality change. --- lib/AST/ASTContext.cpp | 2 ++ lib/IDE/Refactoring.cpp | 10 ++++++---- lib/Serialization/SerializedModuleLoader.cpp | 9 +++------ 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index 14527661a28ea..fc18c3f74da63 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -124,6 +124,8 @@ struct ASTContext::Implementation { /// lazy parsers for imported module files and source files. llvm::SmallPtrSet lazyParsers; + // FIXME: This is a StringMap rather than a StringSet because StringSet + // doesn't allow passing in a pre-existing allocator. llvm::StringMap IdentifierTable; /// The declaration of Swift.AssignmentPrecedence. diff --git a/lib/IDE/Refactoring.cpp b/lib/IDE/Refactoring.cpp index e6dd793c25051..c585133259004 100644 --- a/lib/IDE/Refactoring.cpp +++ b/lib/IDE/Refactoring.cpp @@ -387,7 +387,7 @@ class RenameRangeDetailCollector : public Renamer { }; class TextReplacementsRenamer : public Renamer { - llvm::StringMap &ReplaceTextContext; + llvm::StringSet<> &ReplaceTextContext; std::vector Replacements; public: @@ -395,7 +395,9 @@ class TextReplacementsRenamer : public Renamer { private: StringRef registerText(StringRef Text) { - return ReplaceTextContext.insert({Text, char()}).first->getKey(); + if (Text.empty()) + return Text; + return ReplaceTextContext.insert(Text).first->getKey(); } StringRef getCallArgLabelReplacement(StringRef OldLabelRange, @@ -497,7 +499,7 @@ class TextReplacementsRenamer : public Renamer { public: TextReplacementsRenamer(const SourceManager &SM, StringRef OldName, StringRef NewName, - llvm::StringMap &ReplaceTextContext) + llvm::StringSet<> &ReplaceTextContext) : Renamer(SM, OldName), ReplaceTextContext(ReplaceTextContext), New(NewName) { assert(Old.isValid() && New.isValid()); @@ -3285,7 +3287,7 @@ int swift::ide::syntacticRename(SourceFile *SF, ArrayRef RenameLocs, return true; // Already diagnosed. size_t index = 0; - llvm::StringMap ReplaceTextContext; + llvm::StringSet<> ReplaceTextContext; for(const RenameLoc &Rename: RenameLocs) { ResolvedLoc &Resolved = ResolvedLocs[index++]; TextReplacementsRenamer Renamer(SM, Rename.OldName, Rename.NewName, diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp index 228e53225fb38..09b95d2379475 100644 --- a/lib/Serialization/SerializedModuleLoader.cpp +++ b/lib/Serialization/SerializedModuleLoader.cpp @@ -20,6 +20,7 @@ #include "swift/Basic/SourceManager.h" #include "swift/Basic/Version.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringSet.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" @@ -376,7 +377,7 @@ FileUnit *SerializedModuleLoader::loadAST( // Figure out /which/ dependencies are missing. // FIXME: Dependencies should be de-duplicated at serialization time, // not now. - llvm::StringMap duplicates; + llvm::StringSet<> duplicates; llvm::SmallVector missing; std::copy_if(loadedModuleFile->getDependencies().begin(), loadedModuleFile->getDependencies().end(), @@ -384,11 +385,7 @@ FileUnit *SerializedModuleLoader::loadAST( [&duplicates](const ModuleFile::Dependency &dependency)->bool { if (dependency.isLoaded() || dependency.isHeader()) return false; - bool &seen = duplicates[dependency.RawPath]; - if (seen) - return false; - seen = true; - return true; + return duplicates.insert(dependency.RawPath).second; }); // FIXME: only show module part of RawAccessPath From 71760bcc4ecf892db3ee22dc0994ec4acec83e77 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 13 Sep 2018 11:10:15 -0700 Subject: [PATCH 3/3] Replace StringMap with DenseMap when the keys don't need to be owned StringMap always copies its strings into its own storage. A DenseMap of StringRefs has the same caveats as any other use of StringRef, but in the cases I've changed the string has very clear ownership that outlives the map. No functionality change, but should reduce memory usage and malloc traffic a little. --- include/swift/Basic/SourceManager.h | 4 ++-- lib/ClangImporter/ClangImporter.cpp | 4 ++-- lib/ClangImporter/ImporterImpl.h | 7 ++----- lib/Driver/Driver.cpp | 4 ++-- lib/Driver/ToolChain.cpp | 2 +- lib/Driver/ToolChains.cpp | 2 +- lib/IRGen/IRGen.cpp | 2 +- lib/PrintAsObjC/PrintAsObjC.cpp | 4 ++-- lib/SIL/SILVerifier.cpp | 2 +- 9 files changed, 14 insertions(+), 17 deletions(-) diff --git a/include/swift/Basic/SourceManager.h b/include/swift/Basic/SourceManager.h index 0942e3185b8f7..f952efdacc6cc 100644 --- a/include/swift/Basic/SourceManager.h +++ b/include/swift/Basic/SourceManager.h @@ -31,13 +31,13 @@ class SourceManager { unsigned CodeCompletionOffset; /// Associates buffer identifiers to buffer IDs. - llvm::StringMap BufIdentIDMap; + llvm::DenseMap BufIdentIDMap; /// A cache mapping buffer identifiers to vfs Status entries. /// /// This is as much a hack to prolong the lifetime of status objects as it is /// to speed up stats. - mutable llvm::StringMap StatusCache; + mutable llvm::DenseMap StatusCache; // \c #sourceLocation directive handling. struct VirtualFile { diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index d62958fa82c71..6131a0e47fb18 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -3288,7 +3288,7 @@ bool ClangImporter::Implementation::forEachLookupTable( // Collect and sort the set of module names. SmallVector moduleNames; for (const auto &entry : LookupTables) { - moduleNames.push_back(entry.first()); + moduleNames.push_back(entry.first); } llvm::array_pod_sort(moduleNames.begin(), moduleNames.end()); @@ -3596,7 +3596,7 @@ void ClangImporter::Implementation::dumpSwiftLookupTables() { // Sort the module names so we can print in a deterministic order. SmallVector moduleNames; for (const auto &lookupTable : LookupTables) { - moduleNames.push_back(lookupTable.first()); + moduleNames.push_back(lookupTable.first); } array_pod_sort(moduleNames.begin(), moduleNames.end()); diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h index c91f48c9a3249..0d826d129f34c 100644 --- a/lib/ClangImporter/ImporterImpl.h +++ b/lib/ClangImporter/ImporterImpl.h @@ -279,7 +279,8 @@ struct PlatformAvailability { }; } -using LookupTableMap = llvm::StringMap>; +using LookupTableMap = + llvm::DenseMap>; /// The result of importing a clang type. It holds both the Swift Type /// as well as a bool in which 'true' indicates either: @@ -489,10 +490,6 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation return getNameImporter().getEnumKind(decl); } - // TODO: drop this accessor as soon as we further de-couple the swift name - // lookup tables from the Impl. - LookupTableMap &getLookupTables() { return LookupTables; } - private: /// A mapping from imported declarations to their "alternate" declarations, /// for cases where a single Clang declaration is imported to two diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp index 658539370b581..37ad84ad2a229 100644 --- a/lib/Driver/Driver.cpp +++ b/lib/Driver/Driver.cpp @@ -586,7 +586,7 @@ static bool populateOutOfDateMap(InputInfoMap &map, // If a file was removed, we've lost its dependency info. Rebuild everything. // FIXME: Can we do better? if (ShowIncrementalBuildDecisions) { - llvm::StringSet<> inputArgs; + llvm::DenseSet inputArgs; for (auto &inputPair : inputs) { inputArgs.insert(inputPair.second->getValue()); } @@ -1181,7 +1181,7 @@ static bool checkInputExistence(const Driver &D, const DerivedArgList &Args, void Driver::buildInputs(const ToolChain &TC, const DerivedArgList &Args, InputFileList &Inputs) const { - llvm::StringMap SourceFileNames; + llvm::DenseMap SourceFileNames; for (Arg *A : Args) { if (A->getOption().getKind() == Option::InputClass) { diff --git a/lib/Driver/ToolChain.cpp b/lib/Driver/ToolChain.cpp index dbf283b0a659f..cdabf0cec5300 100644 --- a/lib/Driver/ToolChain.cpp +++ b/lib/Driver/ToolChain.cpp @@ -290,7 +290,7 @@ static void sortJobsToMatchCompilationInputs(ArrayRef unsortedJobs, SmallVectorImpl &sortedJobs, Compilation &C) { - llvm::StringMap jobsByInput; + llvm::DenseMap jobsByInput; for (const Job *J : unsortedJobs) { const CompileJobAction *CJA = cast(&J->getSource()); const InputAction *IA = findSingleSwiftInput(CJA); diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp index 347426d909ca3..e0db0a8407b92 100644 --- a/lib/Driver/ToolChains.cpp +++ b/lib/Driver/ToolChains.cpp @@ -503,7 +503,7 @@ void ToolChain::JobContext::addFrontendCommandLineInputArguments( const bool mayHavePrimaryInputs, const bool useFileList, const bool usePrimaryFileList, const bool filterByType, ArgStringList &arguments) const { - llvm::StringSet<> primaries; + llvm::DenseSet primaries; if (mayHavePrimaryInputs) { for (const Action *A : InputActions) { diff --git a/lib/IRGen/IRGen.cpp b/lib/IRGen/IRGen.cpp index d6a02c6b67a5f..419adb0af6853 100644 --- a/lib/IRGen/IRGen.cpp +++ b/lib/IRGen/IRGen.cpp @@ -1010,7 +1010,7 @@ static void performParallelIRGeneration( PrimaryGM->addLinkLibrary(linkLib); }); - llvm::StringSet<> referencedGlobals; + llvm::DenseSet referencedGlobals; for (auto it = irgen.begin(); it != irgen.end(); ++it) { IRGenModule *IGM = it->second; diff --git a/lib/PrintAsObjC/PrintAsObjC.cpp b/lib/PrintAsObjC/PrintAsObjC.cpp index 434c032b02579..73fc00c570d55 100644 --- a/lib/PrintAsObjC/PrintAsObjC.cpp +++ b/lib/PrintAsObjC/PrintAsObjC.cpp @@ -65,8 +65,8 @@ static bool isAnyObjectOrAny(Type type) { /// Returns true if \p name matches a keyword in any Clang language mode. static bool isClangKeyword(Identifier name) { - static const llvm::StringSet<> keywords = []{ - llvm::StringSet<> set; + static const llvm::DenseSet keywords = []{ + llvm::DenseSet set; // FIXME: clang::IdentifierInfo /nearly/ has the API we need to do this // in a more principled way, but not quite. #define KEYWORD(SPELLING, FLAGS) \ diff --git a/lib/SIL/SILVerifier.cpp b/lib/SIL/SILVerifier.cpp index bd308248b6183..d9e9b59e2d3bb 100644 --- a/lib/SIL/SILVerifier.cpp +++ b/lib/SIL/SILVerifier.cpp @@ -5065,7 +5065,7 @@ void SILModule::verify() const { return; #endif // Uniquing set to catch symbol name collisions. - llvm::StringSet<> symbolNames; + llvm::DenseSet symbolNames; // When merging partial modules, we only link functions from the current // module, without enabling "LinkAll" mode or running the SILLinker pass;