[clang][deps] Use MemoryBuffer in minimizing FS

[jansvoboda11]

Summary

This patch avoids unnecessarily copying contents of mmap-ed files into CachedFileSystemEntry by storing MemoryBuffer instead. The change leads to ~50% reduction of peak memory footprint when scanning LLVM+Clang via clang-scan-deps.

Depends on D115331.

[jansvoboda11]

llvm/include/llvm/Support/SmallVectorMemoryBuffer.h

54 ↗

(On Diff #391643)

I'm not happy with introducing new (hacky) constructor.

But, the best alternative I could come up with is to avoid using SmallVectorMemoryBuffer, store the SmallString with minimized contents somewhere in the filesystem and refer to it via regular MemoryBuffer. This seems needlessly complicated, so hacky constructor it is...

Side question: is the hassle with implicit null-termination (expected by Clang's lexer) worth the complications? What are the benefits anyway?

[dexonsmith]

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

59–62

This is a bit simpler:

Result.Contents = MinimizedFileContents.isSmall()
                    ? MemoryBuffer::getMemBufferCopy(...)
                    : std::make_unique<llvm::SmallVectorMemoryBuffer>();

and doesn't require changing SmallVectorMemoryBuffer.

Another option is:

{
  // Move to the heap and ensure null-terminated to ensure the MemoryBuffer works.
  SmallVector<char, 0> Heap(std::move(MinimizedFileContents));
  Heap.push_back(0);
  Heap.pop_back();
  Result.Contents = std::make_unique<llvm::SmallVectorMemoryBuffer>(std::move(Heap));
}

The latter seems like reasonable logic for the SmallVectorMemoryBuffer constructor though.

llvm/include/llvm/Support/SmallVectorMemoryBuffer.h

54 ↗

(On Diff #391643)

It'd be better to add a RequiresNullTerminator argument to the constructor, matching the MemoryBuffer factory functions, which (if set) does the push_back/pop_back dance, and in either case passes it through to init().

SmallVectorMemoryBuffer(SmallVectorImpl<char> &&SV, bool RequiresNullTerminator = true)
    : SV(std::move(SV)), BufferName("<in-memory object>") {
  if (RequiresNullTerminator) {
    this->SV.push_back(0);
    this->SV.pop_back(0);
  }
  init(this->SV.begin(), this->SV.end(), RequiresNullTerminator);
}

If you do that, you should update the (few) existing callers to explicitly pass false. Note that RequiresNullTerminator defaults to true in all the other MemoryBuffer APIs so it'd be confusing to default it to false here.

Side question: is the hassle with implicit null-termination (expected by Clang's lexer) worth the complications? What are the benefits anyway?

Not sure whether it's a micro-optimization for getting better codegen or if it just predates StringRef, which made it much easier to reason about a byte range as a single unit (rather than tracking two separate pointers, which is more error-prone). I somewhat doubt it's worth it, which is why when I wrote the minimizer I used StringRef, but I don't have data and it'd be a lot of work to change. (The minimizer is fast enough as-is, but it does less work than the full Clang lexer. It's plausible that the optimizations matter there?)

[jansvoboda11]

Rebase on top of D115331.

[jansvoboda11]

llvm/include/llvm/Support/SmallVectorMemoryBuffer.h

54 ↗

(On Diff #391643)

You're right that exposing the RequiresNullTerminator parameter is much clearer than passing in a random lambda. I've extracted the change into D115331.

[jansvoboda11]

Update comment

[jansvoboda11]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

49

In the implementation of this function we get std::unique_ptr<llvm::vfs::File> from the underlying filesystem. When that goes out of scope, the file descriptor will be closed immediately. How did copying contents into memory prevent running out of file descriptors? @arphaman

[dexonsmith]

LGTM.

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

public:
 
   CachedFileSystemEntry(std::error_code Error) : MaybeStat(std::move(Error)) {}
 
   /// Create an entry that represents an opened source file with minimized or
   /// original contents.
   ///
   /// The filesystem opens the file even for `stat` calls open to avoid the
   /// issues with stat + open of minimized files that might lead to a
-  /// mismatching size of the file. If file is not minimized, the full file is
-  /// read and copied into memory to ensure that it's not memory mapped to avoid
-  /// running out of file descriptors.
+  /// mismatching size of the file.
   static CachedFileSystemEntry createFileEntry(StringRef Filename,
                                                llvm::vfs::FileSystem &FS,
                                                bool Minimize = true);
 
   /// Create an entry that represents a directory on the filesystem.
   static CachedFileSystemEntry createDirectoryEntry(llvm::vfs::Status &&Stat);
 
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
   /// \returns True if the current entry points to a directory.
   bool isDirectory() const { return MaybeStat && MaybeStat->isDirectory(); }
 
   /// \returns The error or the file's contents.
   llvm::ErrorOr<StringRef> getContents() const {
     if (!MaybeStat)
       return MaybeStat.getError();
     assert(!MaybeStat->isDirectory() && "not a file");
     assert(isValid() && "not initialized");
-    return Contents.str();
+    return Contents->getBuffer();
   }
 
   /// \returns The error or the status of the entry.
   llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
     assert(isValid() && "not initialized");
     return MaybeStat;
   }
 
public:
   CachedFileSystemEntry(CachedFileSystemEntry &&) = default;
   CachedFileSystemEntry &operator=(CachedFileSystemEntry &&) = default;
 
   CachedFileSystemEntry(const CachedFileSystemEntry &) = delete;
   CachedFileSystemEntry &operator=(const CachedFileSystemEntry &) = delete;
 
 private:
   llvm::ErrorOr<llvm::vfs::Status> MaybeStat;
-  // Store the contents in a small string to allow a
-  // move from the small string for the minimized contents.
-  // Note: small size of 1 allows us to store an empty string with an implicit
-  // null terminator without any allocations.
-  llvm::SmallString<1> Contents;
+  std::unique_ptr<llvm::MemoryBuffer> Contents;
   PreprocessorSkippedRangeMapping PPSkippedRangeMapping;
 };
 
 /// This class is a shared cache, that caches the 'stat' and 'open' calls to the
 /// underlying real file system. It distinguishes between minimized and original
 /// files.
 ///
 /// It is sharded based on the hash of the key to reduce the lock contention for

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 //===- DependencyScanningFilesystem.cpp - clang-scan-deps fs --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "clang/Lex/DependencyDirectivesSourceMinimizer.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
     StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize) {
CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
   SmallVector<minimize_source_to_dependency_directives::Token, 64> Tokens;
   if (!Minimize || minimizeSourceToDependencyDirectives(
                        Buffer->getBuffer(), MinimizedFileContents, Tokens)) {
     // Use the original file unless requested otherwise, or
     // if the minimization failed.
     // FIXME: Propage the diagnostic if desired by the client.
     CachedFileSystemEntry Result;
     Result.MaybeStat = std::move(*Stat);
-    Result.Contents.reserve(Buffer->getBufferSize() + 1);
-    Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
-    // Implicitly null terminate the contents for Clang's lexer.
-    Result.Contents.push_back('\0');
-    Result.Contents.pop_back();
+    Result.Contents = std::move(*MaybeBuffer);
     return Result;
   }
 
   CachedFileSystemEntry Result;
   size_t Size = MinimizedFileContents.size();
   Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
                                        Stat->getLastModificationTime(),
                                        Stat->getUser(), Stat->getGroup(), Size,
                                        Stat->getType(), Stat->getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
          "not null terminated contents");
-  // Even though there's an implicit null terminator in the minimized contents,
-  // we want to temporarily make it explicit. This will ensure that the
-  // std::move will preserve it even if it needs to do a copy if the
-  // SmallString still has the small capacity.
-  MinimizedFileContents.push_back('\0');
-  Result.Contents = std::move(MinimizedFileContents);
-  // Now make the null terminator implicit again, so that Clang's lexer can find
-  // it right where the buffer ends.
-  Result.Contents.pop_back();
+  Result.Contents = std::make_unique<llvm::SmallVectorMemoryBuffer>(
+      std::move(MinimizedFileContents));
 
   // Compute the skipped PP ranges that speedup skipping over inactive
   // preprocessor blocks.
   llvm::SmallVector<minimize_source_to_dependency_directives::SkippedRange, 32>
       SkippedRanges;
   minimize_source_to_dependency_directives::computeSkippedRanges(Tokens,
                                                                  SkippedRanges);
   PreprocessorSkippedRangeMapping Mapping;
   for (const auto &Range : SkippedRanges) {