On Oct 29, 2011, at 8:02 AM, Ted Kremenek wrote: > Can't the preamble be big? Unless I'm mistaken, aren't you talking about > multiple megabytes here (especially when the original project isn't using > PCH)? Another thing to consider is that there may be multiple ASTUnits in > use at the same time.
We are creating the file only to immediately map it back in memory, and that file is not getting any re-use (I mean only one ASTUnit will use it so the memory is not getting shared). Is this scheme more memory efficient ? Maybe chunks of the file end up not needed to be brought in memory ? We should probably investigate whether there is actual benefit with using files here. > > Sent from my iPad > > On Oct 28, 2011, at 9:07 PM, Argyrios Kyrtzidis <[email protected]> wrote: > >> On Oct 27, 2011, at 10:55 AM, Ted Kremenek wrote: >> >>> Author: kremenek >>> Date: Thu Oct 27 12:55:18 2011 >>> New Revision: 143115 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=143115&view=rev >>> Log: >>> Move ASTUnit's handling of temporary files and the preamble file into a >>> lazily-created static DenseMap. This DenseMap is cleared (and the files >>> erased) via an atexit routine in the case an ASTUnit is not destroyed. >>> Fixes <rdar://problem/10293367>. >> >> How about storing the preamble PCH in memory in the corresponding ASTUnit >> instead ? >> There doesn't seem to be any benefits in creating a file for it, plus we >> save on the I/O and the worry of cleaning up behind us. >> >> -Argyrios >> >>> >>> Modified: >>> cfe/trunk/include/clang/Frontend/ASTUnit.h >>> cfe/trunk/lib/Frontend/ASTUnit.cpp >>> cfe/trunk/test/Index/crash-recovery-code-complete.c >>> cfe/trunk/test/Index/crash-recovery-reparse.c >>> >>> Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=143115&r1=143114&r2=143115&view=diff >>> ============================================================================== >>> --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original) >>> +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Thu Oct 27 12:55:18 2011 >>> @@ -147,10 +147,6 @@ >>> /// the next. >>> unsigned NumStoredDiagnosticsFromDriver; >>> >>> - /// \brief Temporary files that should be removed when the ASTUnit is >>> - /// destroyed. >>> - SmallVector<llvm::sys::Path, 4> TemporaryFiles; >>> - >>> /// \brief Counter that determines when we want to try building a >>> /// precompiled preamble. >>> /// >>> @@ -161,10 +157,7 @@ >>> /// building the precompiled preamble fails, we won't try again for >>> /// some number of calls. >>> unsigned PreambleRebuildCounter; >>> - >>> - /// \brief The file in which the precompiled preamble is stored. >>> - std::string PreambleFile; >>> - >>> + >>> public: >>> class PreambleData { >>> const FileEntry *File; >>> @@ -468,9 +461,7 @@ >>> /// \brief Add a temporary file that the ASTUnit depends on. >>> /// >>> /// This file will be erased when the ASTUnit is destroyed. >>> - void addTemporaryFile(const llvm::sys::Path &TempFile) { >>> - TemporaryFiles.push_back(TempFile); >>> - } >>> + void addTemporaryFile(const llvm::sys::Path &TempFile); >>> >>> bool getOnlyLocalDecls() const { return OnlyLocalDecls; } >>> >>> >>> Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=143115&r1=143114&r2=143115&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original) >>> +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Thu Oct 27 12:55:18 2011 >>> @@ -81,6 +81,102 @@ >>> } >>> } >>> }; >>> + >>> + struct OnDiskData { >>> + /// \brief The file in which the precompiled preamble is stored. >>> + std::string PreambleFile; >>> + >>> + /// \brief Temporary files that should be removed when the ASTUnit is >>> + /// destroyed. >>> + SmallVector<llvm::sys::Path, 4> TemporaryFiles; >>> + >>> + /// \brief Erase temporary files. >>> + void CleanTemporaryFiles(); >>> + >>> + /// \brief Erase the preamble file. >>> + void CleanPreambleFile(); >>> + >>> + /// \brief Erase temporary files and the preamble file. >>> + void Cleanup(); >>> + }; >>> +} >>> + >>> +static void cleanupOnDiskMapAtExit(void); >>> + >>> +typedef llvm::DenseMap<const ASTUnit *, OnDiskData *> OnDiskDataMap; >>> +static OnDiskDataMap &getOnDiskDataMap() { >>> + static OnDiskDataMap M; >>> + static bool hasRegisteredAtExit = false; >>> + if (!hasRegisteredAtExit) { >>> + hasRegisteredAtExit = true; >>> + atexit(cleanupOnDiskMapAtExit); >>> + } >>> + return M; >>> +} >>> + >>> +static void cleanupOnDiskMapAtExit(void) { >>> + OnDiskDataMap &M = getOnDiskDataMap(); >>> + for (OnDiskDataMap::iterator I = M.begin(), E = M.end(); I != E; ++I) { >>> + // We don't worry about freeing the memory associated with >>> OnDiskDataMap. >>> + // All we care about is erasing stale files. >>> + I->second->Cleanup(); >>> + } >>> +} >>> + >>> +static OnDiskData &getOnDiskData(const ASTUnit *AU) { >>> + OnDiskDataMap &M = getOnDiskDataMap(); >>> + OnDiskData *&D = M[AU]; >>> + if (!D) >>> + D = new OnDiskData(); >>> + return *D; >>> +} >>> + >>> +static void erasePreambleFile(const ASTUnit *AU) { >>> + getOnDiskData(AU).CleanPreambleFile(); >>> +} >>> + >>> +static void removeOnDiskEntry(const ASTUnit *AU) { >>> + OnDiskDataMap &M = getOnDiskDataMap(); >>> + OnDiskDataMap::iterator I = M.find(AU); >>> + if (I != M.end()) { >>> + I->second->Cleanup(); >>> + delete I->second; >>> + M.erase(AU); >>> + } >>> +} >>> + >>> +static void setPreambleFile(const ASTUnit *AU, llvm::StringRef >>> preambleFile) { >>> + getOnDiskData(AU).PreambleFile = preambleFile; >>> +} >>> + >>> +static const std::string &getPreambleFile(const ASTUnit *AU) { >>> + return getOnDiskData(AU).PreambleFile; >>> +} >>> + >>> +void OnDiskData::CleanTemporaryFiles() { >>> + for (unsigned I = 0, N = TemporaryFiles.size(); I != N; ++I) >>> + TemporaryFiles[I].eraseFromDisk(); >>> + TemporaryFiles.clear(); >>> +} >>> + >>> +void OnDiskData::CleanPreambleFile() { >>> + if (!PreambleFile.empty()) { >>> + llvm::sys::Path(PreambleFile).eraseFromDisk(); >>> + PreambleFile.clear(); >>> + } >>> +} >>> + >>> +void OnDiskData::Cleanup() { >>> + CleanTemporaryFiles(); >>> + CleanPreambleFile(); >>> +} >>> + >>> +void ASTUnit::CleanTemporaryFiles() { >>> + getOnDiskData(this).CleanTemporaryFiles(); >>> +} >>> + >>> +void ASTUnit::addTemporaryFile(const llvm::sys::Path &TempFile) { >>> + getOnDiskData(this).TemporaryFiles.push_back(TempFile); >>> } >>> >>> /// \brief After failing to build a precompiled preamble (due to >>> @@ -114,10 +210,9 @@ >>> } >>> >>> ASTUnit::~ASTUnit() { >>> - CleanTemporaryFiles(); >>> - if (!PreambleFile.empty()) >>> - llvm::sys::Path(PreambleFile).eraseFromDisk(); >>> - >>> + // Clean up the temporary files and the preamble file. >>> + removeOnDiskEntry(this); >>> + >>> // Free the buffers associated with remapped files. We are required to >>> // perform this operation here because we explicitly request that the >>> // compiler instance *not* free these buffers for each invocation of the >>> @@ -143,12 +238,6 @@ >>> } >>> } >>> >>> -void ASTUnit::CleanTemporaryFiles() { >>> - for (unsigned I = 0, N = TemporaryFiles.size(); I != N; ++I) >>> - TemporaryFiles[I].eraseFromDisk(); >>> - TemporaryFiles.clear(); >>> -} >>> - >>> /// \brief Determine the set of code-completion contexts in which this >>> /// declaration should be shown. >>> static unsigned getDeclShowContexts(NamedDecl *ND, >>> @@ -939,7 +1028,7 @@ >>> PreprocessorOpts.PrecompiledPreambleBytes.first = Preamble.size(); >>> PreprocessorOpts.PrecompiledPreambleBytes.second >>> = >>> PreambleEndsAtStartOfLine; >>> - PreprocessorOpts.ImplicitPCHInclude = PreambleFile; >>> + PreprocessorOpts.ImplicitPCHInclude = getPreambleFile(this); >>> PreprocessorOpts.DisablePCHValidation = true; >>> >>> // The stored diagnostic has the old source manager in it; update >>> @@ -971,7 +1060,7 @@ >>> goto error; >>> >>> if (OverrideMainBuffer) { >>> - std::string ModName = PreambleFile; >>> + std::string ModName = getPreambleFile(this); >>> TranslateStoredDiagnostics(Clang->getModuleManager(), ModName, >>> getSourceManager(), PreambleDiagnostics, >>> StoredDiagnostics); >>> @@ -1172,10 +1261,7 @@ >>> // We couldn't find a preamble in the main source. Clear out the current >>> // preamble, if we have one. It's obviously no good any more. >>> Preamble.clear(); >>> - if (!PreambleFile.empty()) { >>> - llvm::sys::Path(PreambleFile).eraseFromDisk(); >>> - PreambleFile.clear(); >>> - } >>> + erasePreambleFile(this); >>> >>> // The next time we actually see a preamble, precompile it. >>> PreambleRebuildCounter = 1; >>> @@ -1281,7 +1367,7 @@ >>> // We can't reuse the previously-computed preamble. Build a new one. >>> Preamble.clear(); >>> PreambleDiagnostics.clear(); >>> - llvm::sys::Path(PreambleFile).eraseFromDisk(); >>> + erasePreambleFile(this); >>> PreambleRebuildCounter = 1; >>> } else if (!AllowRebuild) { >>> // We aren't allowed to rebuild the precompiled preamble; just >>> @@ -1439,7 +1525,7 @@ >>> StoredDiagnostics.erase(stored_diag_afterDriver_begin(), stored_diag_end()); >>> >>> // Keep track of the preamble we precompiled. >>> - PreambleFile = FrontendOpts.OutputFile; >>> + setPreambleFile(this, FrontendOpts.OutputFile); >>> NumWarningsInPreamble = getDiagnostics().getNumWarnings(); >>> >>> // Keep track of all of the files that the source manager knows about, >>> @@ -1802,7 +1888,7 @@ >>> // If we have a preamble file lying around, or if we might try to >>> // build a precompiled preamble, do so now. >>> llvm::MemoryBuffer *OverrideMainBuffer = 0; >>> - if (!PreambleFile.empty() || PreambleRebuildCounter > 0) >>> + if (!getPreambleFile(this).empty() || PreambleRebuildCounter > 0) >>> OverrideMainBuffer = getMainBufferWithPrecompiledPreamble(*Invocation); >>> >>> // Clear out the diagnostics state. >>> @@ -2173,7 +2259,7 @@ >>> // point is within the main file, after the end of the precompiled >>> // preamble. >>> llvm::MemoryBuffer *OverrideMainBuffer = 0; >>> - if (!PreambleFile.empty()) { >>> + if (!getPreambleFile(this).empty()) { >>> using llvm::sys::FileStatus; >>> llvm::sys::PathWithStatus CompleteFilePath(File); >>> llvm::sys::PathWithStatus MainPath(OriginalSourceFile); >>> @@ -2197,7 +2283,7 @@ >>> PreprocessorOpts.PrecompiledPreambleBytes.first = Preamble.size(); >>> PreprocessorOpts.PrecompiledPreambleBytes.second >>> = >>> PreambleEndsAtStartOfLine; >>> - PreprocessorOpts.ImplicitPCHInclude = PreambleFile; >>> + PreprocessorOpts.ImplicitPCHInclude = getPreambleFile(this); >>> PreprocessorOpts.DisablePCHValidation = true; >>> >>> OwnedBuffers.push_back(OverrideMainBuffer); >>> @@ -2214,7 +2300,7 @@ >>> if (Act->BeginSourceFile(*Clang.get(), >>> Clang->getFrontendOpts().Inputs[0].second, >>> Clang->getFrontendOpts().Inputs[0].first)) { >>> if (OverrideMainBuffer) { >>> - std::string ModName = PreambleFile; >>> + std::string ModName = getPreambleFile(this); >>> TranslateStoredDiagnostics(Clang->getModuleManager(), ModName, >>> getSourceManager(), PreambleDiagnostics, >>> StoredDiagnostics); >>> >>> Modified: cfe/trunk/test/Index/crash-recovery-code-complete.c >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/crash-recovery-code-complete.c?rev=143115&r1=143114&r2=143115&view=diff >>> ============================================================================== >>> --- cfe/trunk/test/Index/crash-recovery-code-complete.c (original) >>> +++ cfe/trunk/test/Index/crash-recovery-code-complete.c Thu Oct 27 12:55:18 >>> 2011 >>> @@ -3,7 +3,7 @@ >>> // RUN: "-remap-file=%s;%S/Inputs/crash-recovery-code-complete-remap.c" \ >>> // RUN: %s 2> %t.err >>> // RUN: FileCheck < %t.err -check-prefix=CHECK-CODE-COMPLETE-CRASH %s >>> -// RUN: rm %t-preamble.pch >>> +// RUN: test ! -e %t-preamble.pch >>> // CHECK-CODE-COMPLETE-CRASH: Unable to perform code completion! >>> // >>> // REQUIRES: crash-recovery >>> >>> Modified: cfe/trunk/test/Index/crash-recovery-reparse.c >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/crash-recovery-reparse.c?rev=143115&r1=143114&r2=143115&view=diff >>> ============================================================================== >>> --- cfe/trunk/test/Index/crash-recovery-reparse.c (original) >>> +++ cfe/trunk/test/Index/crash-recovery-reparse.c Thu Oct 27 12:55:18 2011 >>> @@ -3,7 +3,7 @@ >>> // RUN: -remap-file="%s;%S/Inputs/crash-recovery-reparse-remap.c" \ >>> // RUN: %s 2> %t.err >>> // RUN: FileCheck < %t.err -check-prefix=CHECK-REPARSE-SOURCE-CRASH %s >>> -// RUN: rm %t-preamble.pch >>> +// RUN: test ! -e $t-preamble.pch >>> // CHECK-REPARSE-SOURCE-CRASH: Unable to reparse translation unit >>> // >>> // REQUIRES: crash-recovery >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
