Page MenuHomePhabricator

Close FileEntries of cached files in ModuleManager::addModule().
ClosedPublic

Authored by aprantl on Aug 16 2018, 2:56 PM.

Details

Summary

Close FileEntries of cached files in ModuleManager::addModule().

While investigating why LLDB (which can build hundreds of clang
modules during one debug session) was getting "too many open files"
errors, I found that most of them are .pcm files that are kept open by
ModuleManager. Pretty much all of the open file dscriptors are
FileEntries that are refering to .pcm files for which a buffer
already exists in a CompilerInstance's PCMCache.

Before PCMCache was added it was necessary to hold on to open file
descriptors to ensure that all ModuleManagers using the same
FileManager read the a consistent version of a given .pcm file on
disk, even when a concurrent clang process overwrites the file halfway
through. The PCMCache makes this practice unnecessary, since it caches
the entire contents of a .pcm file, while the FileManager caches all
the stat() information.

This patch adds a call to FileEntry::closeFile() to the path where a
Buffer has already been created. This is necessary because even for a
freshly written .pcm file the file is stat()ed once immediately
after writing to generate a FileEntry in the FileManager. Because a
freshly-generated file's contents is stored in the PCMCache, it is
fine to close the file immediately thereafter. The second change this
patch makes is to set the ShouldClose flag to true when reading a
.pcm file into the PCMCache for the first time.

[For reference, in 1 Clang instance there is

  • 1 FileManager and
  • n ModuleManagers with
  • n PCMCaches.]

rdar://problem/40906753

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Aug 16 2018, 2:56 PM

@bruno: When we last discussed this my plan was to avoid the stat() in lookupModuleFile() for files that were just added to the PCMCache by WriteAST() entirely, but ModuleManager::Modules is a DenseMap<FileEntry, ModuleFile *> and lookupModuleFile() is the easiest way to create a new FileEntry. It would be nice to find a way to avoid the stat() for a file that we just wrote, but it wasn't immediately obvious to me how to do that.

bruno accepted this revision.Aug 17 2018, 6:16 PM

Thanks for working on this Adrian, LGTM.

When we last discussed this my plan was to avoid the stat() in lookupModuleFile() for files that were just added to the PCMCache by WriteAST() entirely, but ModuleManager::Modules is a DenseMap<FileEntry, ModuleFile *> and lookupModuleFile() is the easiest way to create a new FileEntry. It would be nice to find a way to avoid the stat() for a file that we just wrote, but it wasn't immediately obvious to me how to do that.

Makes sense, I don't know offhand either. It's an extra optimization step we want to do, but it shouldn't block this change.

This revision is now accepted and ready to land.Aug 17 2018, 6:16 PM
This revision was automatically updated to reflect the committed changes.