[cfe-commits] r143115 - in /cfe/trunk: include/clang/Frontend/ASTUnit.h lib/Frontend/ASTUnit.cpp test/Index/crash-recovery-code-complete.c test/Index/crash-recovery-reparse.c
Ted Kremenek
kremenek at apple.com
Sat Oct 29 11:46:09 PDT 2011
I see this as having the same performance characteristics as PCH. What you are proposing would be to keep the entire PCH blob in memory. That inherently raises the high water mark for memory usage of clients of ASTUnit for the entire time they are using the ASTUnit.
On Oct 29, 2011, at 11:24 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 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 <akyrtzi at gmail.com> 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
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>
More information about the cfe-commits
mailing list