[PATCH] D98364: [clangd] Use ref counted strings throughout for File Contents

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Avoiding copies seems nice, but this makes some interfaces more awkward. Do you have any measurements that some of these copies matter?

(The dirty FS changes avoided coping all the dirty buffers at once, but it seems like these changes will mostly save ~1 file copy per operation)



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:190
   /// diagnostics etc built from it. If empty, a version number is generated.
+  void addDocument(PathRef File, std::shared_ptr<const std::string> Contents,
+                   llvm::StringRef Version = "null",
----------------
what's the purpose of changing this interface?

The only callers that matter are in ClangdLSPServer, and this signature requires it to copy anyway. So why not copy inside ClangdServer?

(StringRef is more convenient to for tests and C++ embedders, and just a bit less churn)


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:231
 llvm::Expected<ScannedPreamble>
-scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
+scanPreamble(std::shared_ptr<const std::string> Contents,
+             const tooling::CompileCommand &Cmd) {
----------------
I don't think this change makes sense, we don't need ownership here


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:324
   // without those.
-  auto ContentsBuffer =
-      llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
+  auto ContentsBuffer = getSharedStringBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
----------------
again, no need for ownership


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:376
                           const CompilerInvocation &CI) {
-  auto ContentsBuffer =
-      llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
+  auto ContentsBuffer = getSharedStringBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
----------------
and here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98364/new/

https://reviews.llvm.org/D98364