[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0250b053b5aa: [clangd] Add a Filesystem that overlays Dirty 
files. (authored by njames93).

Changed prior to commit:
  https://reviews.llvm.org/D94554?vs=329070=329319#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DraftStore.cpp
  clang-tools-extra/clangd/DraftStore.h
  clang-tools-extra/clangd/unittests/DraftStoreTests.cpp

Index: clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
===
--- clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
+++ clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
@@ -24,20 +24,20 @@
 
   EXPECT_EQ("25", DS.addDraft(File, "25", ""));
   EXPECT_EQ("25", DS.getDraft(File)->Version);
-  EXPECT_EQ("", DS.getDraft(File)->Contents);
+  EXPECT_EQ("", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("26", DS.addDraft(File, "", "x"));
   EXPECT_EQ("26", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("27", DS.addDraft(File, "", "x")) << "no-op change";
   EXPECT_EQ("27", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   // We allow versions to go backwards.
   EXPECT_EQ("7", DS.addDraft(File, "7", "y"));
   EXPECT_EQ("7", DS.getDraft(File)->Version);
-  EXPECT_EQ("y", DS.getDraft(File)->Contents);
+  EXPECT_EQ("y", *DS.getDraft(File)->Contents);
 }
 
 } // namespace
Index: clang-tools-extra/clangd/DraftStore.h
===
--- clang-tools-extra/clangd/DraftStore.h
+++ clang-tools-extra/clangd/DraftStore.h
@@ -13,6 +13,7 @@
 #include "support/Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include 
 #include 
 #include 
@@ -27,7 +28,7 @@
 class DraftStore {
 public:
   struct Draft {
-std::string Contents;
+std::shared_ptr Contents;
 std::string Version;
   };
 
@@ -47,9 +48,15 @@
   /// Remove the draft from the store.
   void removeDraft(PathRef File);
 
+  llvm::IntrusiveRefCntPtr asVFS() const;
+
 private:
+  struct DraftAndTime {
+Draft Draft;
+std::time_t MTime;
+  };
   mutable std::mutex Mutex;
-  llvm::StringMap Drafts;
+  llvm::StringMap Drafts;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/DraftStore.cpp
===
--- clang-tools-extra/clangd/DraftStore.cpp
+++ clang-tools-extra/clangd/DraftStore.cpp
@@ -11,6 +11,8 @@
 #include "support/Logger.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -22,7 +24,7 @@
   if (It == Drafts.end())
 return None;
 
-  return It->second;
+  return It->second.Draft;
 }
 
 std::vector DraftStore::getActiveFiles() const {
@@ -75,10 +77,11 @@
  llvm::StringRef Contents) {
   std::lock_guard Lock(Mutex);
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto  = Drafts[File];
+  updateVersion(D.Draft, Version);
+  std::time();
+  D.Draft.Contents = std::make_shared(Contents);
+  return D.Draft.Version;
 }
 
 void DraftStore::removeDraft(PathRef File) {
@@ -87,5 +90,39 @@
   Drafts.erase(File);
 }
 
+namespace {
+
+/// A read only MemoryBuffer shares ownership of a ref counted string. The
+/// shared string object must not be modified while an owned by this buffer.
+class SharedStringBuffer : public llvm::MemoryBuffer {
+  const std::shared_ptr BufferContents;
+  const std::string Name;
+
+public:
+  BufferKind getBufferKind() const override {
+return MemoryBuffer::MemoryBuffer_Malloc;
+  }
+
+  StringRef getBufferIdentifier() const override { return Name; }
+
+  SharedStringBuffer(std::shared_ptr Data, StringRef Name)
+  : BufferContents(std::move(Data)), Name(Name) {
+assert(BufferContents && "Can't create from empty shared_ptr");
+MemoryBuffer::init(BufferContents->c_str(),
+   BufferContents->c_str() + BufferContents->size(),
+   /*RequiresNullTerminator=*/true);
+  }
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr DraftStore::asVFS() const {
+  auto MemFS = llvm::makeIntrusiveRefCnt();
+  std::lock_guard Guard(Mutex);
+  for (const auto  : Drafts)
+MemFS->addFile(Draft.getKey(), Draft.getValue().MTime,
+   std::make_unique(
+   Draft.getValue().Draft.Contents, Draft.getKey()));
+  return MemFS;
+}
 } // namespace clangd
 } 

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Still LG btw!




Comment at: clang-tools-extra/clangd/ClangdServer.h:342
 
-  llvm::Optional getDraft(PathRef File) const;
+  std::shared_ptr getDraft(PathRef File) const;
 

Maybe add an explicit comment that this returns nullptr if the file is not open.
(When the type was Optional this was fairly self-explanatory)



Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+  const std::unique_ptr DirtyFS;
 };

nit: we don't use const on pointer members, just on the pointee where 
appropriate.
Maybe not the best style in a vacuum but it's fairly consistent and diverging 
from it is a bit confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

njames93 wrote:
> njames93 wrote:
> > sammccall wrote:
> > > njames93 wrote:
> > > > sammccall wrote:
> > > > > doing IO in view() doesn't seem obviously OK.
> > > > > 
> > > > > what's the practical consequence of the overlay's inodes not matching 
> > > > > that of the underlying FS? (It seems reasonable to say that the files 
> > > > > always have different identity, and may/may not have the same content)
> > > > It's probably not necessary, but I know preambles use the UniqueID. It 
> > > > may just be safer to create a uniqueID for each item in the DraftStore 
> > > > and leave it at that.
> > > The biggest risks I can see with this approach:
> > >  - in build-preamble-from-dirty-FS mode, would opening a draft cause the 
> > > preamble to be considered invalid and needing rebuilding? My read of 
> > > PrecompiledPreamble::CanReuse is no (UniqueIDs are not stored)
> > >  - when parsing, if the file can be read through the overlay *and* 
> > > underlying via different paths (e.g. hardlinks), these won't be 
> > > deduplicated. However giving them the same inode only changes the nature 
> > > of the bug: the file would get whichever content was read first.
> > > 
> > > So I think it's as good as we can expect.
> > Unfortunately in the current implementation, opening a draft will 
> > invalidate and preambles that use the underlying file. 
> > This is due to the last modification times not matching. 
> > This could be addressed by querying the TFS on the textDocument/didOpen 
> > request to get the actual last modification time, but we shouldn't really 
> > be doing any kind of IO on that thread.
> @sammccall Any further comments with regard to this. Is calling IO to get 
> modification time in didOpen ok, or just accept opening a file would 
> invalidate a preamble if the DirtyFS was used for preambles.
There's no urgency in resolving this:
 - preambles-from-dirty-fs doesn't exist in this patch
 - even once it does, we can introduce the problem first (in this experimental 
mode) and then solve it

That said, my suspicion is that having ClangdLSPServer stat the file, 
conditional on being in preambles-from-dirty-fs mode, would be OK.
(If the filesize matches I think it's safe to say the content does, and copy 
the timestamp. If the filesize mismatches... well, there are a number of 
scenarios involving encodings, and it's hard to say which will matter in 
practice, so we'd probably want to log and wait for the bug reports)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 329070.
njames93 marked 2 inline comments as done.
njames93 added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DraftStore.cpp
  clang-tools-extra/clangd/DraftStore.h
  clang-tools-extra/clangd/unittests/DraftStoreTests.cpp

Index: clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
===
--- clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
+++ clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
@@ -24,20 +24,20 @@
 
   EXPECT_EQ("25", DS.addDraft(File, "25", ""));
   EXPECT_EQ("25", DS.getDraft(File)->Version);
-  EXPECT_EQ("", DS.getDraft(File)->Contents);
+  EXPECT_EQ("", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("26", DS.addDraft(File, "", "x"));
   EXPECT_EQ("26", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("27", DS.addDraft(File, "", "x")) << "no-op change";
   EXPECT_EQ("27", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   // We allow versions to go backwards.
   EXPECT_EQ("7", DS.addDraft(File, "7", "y"));
   EXPECT_EQ("7", DS.getDraft(File)->Version);
-  EXPECT_EQ("y", DS.getDraft(File)->Contents);
+  EXPECT_EQ("y", *DS.getDraft(File)->Contents);
 }
 
 } // namespace
Index: clang-tools-extra/clangd/DraftStore.h
===
--- clang-tools-extra/clangd/DraftStore.h
+++ clang-tools-extra/clangd/DraftStore.h
@@ -13,6 +13,7 @@
 #include "support/Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include 
 #include 
 #include 
@@ -27,7 +28,7 @@
 class DraftStore {
 public:
   struct Draft {
-std::string Contents;
+std::shared_ptr Contents;
 std::string Version;
   };
 
@@ -47,9 +48,15 @@
   /// Remove the draft from the store.
   void removeDraft(PathRef File);
 
+  llvm::IntrusiveRefCntPtr asVFS() const;
+
 private:
+  struct DraftAndTime {
+Draft Draft;
+std::time_t MTime;
+  };
   mutable std::mutex Mutex;
-  llvm::StringMap Drafts;
+  llvm::StringMap Drafts;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/DraftStore.cpp
===
--- clang-tools-extra/clangd/DraftStore.cpp
+++ clang-tools-extra/clangd/DraftStore.cpp
@@ -11,6 +11,8 @@
 #include "support/Logger.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -22,7 +24,7 @@
   if (It == Drafts.end())
 return None;
 
-  return It->second;
+  return It->second.Draft;
 }
 
 std::vector DraftStore::getActiveFiles() const {
@@ -75,10 +77,11 @@
  llvm::StringRef Contents) {
   std::lock_guard Lock(Mutex);
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto  = Drafts[File];
+  updateVersion(D.Draft, Version);
+  std::time();
+  D.Draft.Contents = std::make_shared(Contents);
+  return D.Draft.Version;
 }
 
 void DraftStore::removeDraft(PathRef File) {
@@ -87,5 +90,39 @@
   Drafts.erase(File);
 }
 
+namespace {
+
+/// A read only MemoryBuffer shares ownership of a ref counted string. The
+/// shared string object must not be modified while an owned by this buffer.
+class SharedStringBuffer : public llvm::MemoryBuffer {
+  const std::shared_ptr BufferContents;
+  const std::string Name;
+
+public:
+  BufferKind getBufferKind() const override {
+return MemoryBuffer::MemoryBuffer_Malloc;
+  }
+
+  StringRef getBufferIdentifier() const override { return Name; }
+
+  SharedStringBuffer(std::shared_ptr Data, StringRef Name)
+  : BufferContents(std::move(Data)), Name(Name) {
+assert(BufferContents && "Can't create from empty shared_ptr");
+MemoryBuffer::init(BufferContents->c_str(),
+   BufferContents->c_str() + BufferContents->size(),
+   /*RequiresNullTerminator=*/true);
+  }
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr DraftStore::asVFS() const {
+  auto MemFS = llvm::makeIntrusiveRefCnt();
+  std::lock_guard Guard(Mutex);
+  for (const auto  : Drafts)
+MemFS->addFile(Draft.getKey(), Draft.getValue().MTime,
+   std::make_unique(
+   Draft.getValue().Draft.Contents, Draft.getKey()));
+  return MemFS;
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 marked 11 inline comments as done.
njames93 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+  class DirtyFS : public ThreadsafeFS {
+  public:

sammccall wrote:
> njames93 wrote:
> > Probably needs moving to its own place, but wasn't sure where best to put 
> > it.
> I think ClangdServer is fine, but this can be a unique_ptr with 
> the class moved to the implementation file
How about privately inheriting from ThreadsafeFS :P
I agree, hiding the impl in the cpp is probably a much nicer way.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:80
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto Res = Drafts.try_emplace(File);
+  auto  = Res.first->getValue();

sammccall wrote:
> we don't need try_emplace here, we overwrite the whole value regardless.
> Can we use `Draft  = Drafts[File]` again?
I changed this because in an older version I wanted to set the UniqueID on the 
first time the file was added, however now we don't do that (or check the 
return value for insertion at all) its safe to go back to the `operator[]` 
access.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

njames93 wrote:
> sammccall wrote:
> > njames93 wrote:
> > > sammccall wrote:
> > > > doing IO in view() doesn't seem obviously OK.
> > > > 
> > > > what's the practical consequence of the overlay's inodes not matching 
> > > > that of the underlying FS? (It seems reasonable to say that the files 
> > > > always have different identity, and may/may not have the same content)
> > > It's probably not necessary, but I know preambles use the UniqueID. It 
> > > may just be safer to create a uniqueID for each item in the DraftStore 
> > > and leave it at that.
> > The biggest risks I can see with this approach:
> >  - in build-preamble-from-dirty-FS mode, would opening a draft cause the 
> > preamble to be considered invalid and needing rebuilding? My read of 
> > PrecompiledPreamble::CanReuse is no (UniqueIDs are not stored)
> >  - when parsing, if the file can be read through the overlay *and* 
> > underlying via different paths (e.g. hardlinks), these won't be 
> > deduplicated. However giving them the same inode only changes the nature of 
> > the bug: the file would get whichever content was read first.
> > 
> > So I think it's as good as we can expect.
> Unfortunately in the current implementation, opening a draft will invalidate 
> and preambles that use the underlying file. 
> This is due to the last modification times not matching. 
> This could be addressed by querying the TFS on the textDocument/didOpen 
> request to get the actual last modification time, but we shouldn't really be 
> doing any kind of IO on that thread.
@sammccall Any further comments with regard to this. Is calling IO to get 
modification time in didOpen ok, or just accept opening a file would invalidate 
a preamble if the DirtyFS was used for preambles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks, LG!




Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+  class DirtyFS : public ThreadsafeFS {
+  public:

njames93 wrote:
> Probably needs moving to its own place, but wasn't sure where best to put it.
I think ClangdServer is fine, but this can be a unique_ptr with 
the class moved to the implementation file



Comment at: clang-tools-extra/clangd/DraftStore.cpp:80
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto Res = Drafts.try_emplace(File);
+  auto  = Res.first->getValue();

we don't need try_emplace here, we overwrite the whole value regardless.
Can we use `Draft  = Drafts[File]` again?



Comment at: clang-tools-extra/clangd/DraftStore.cpp:83
+  updateVersion(D.Draft, Version);
+  time();
+  D.Draft.Contents = std::make_shared(Contents);

nit: std::time() (and std::time_t)



Comment at: clang-tools-extra/clangd/DraftStore.cpp:113
+MemoryBuffer::init(BufferContents->c_str(),
+   BufferContents->c_str() + BufferContents->size(), true);
+  }

nit: /*RequiresNullTerminator=*/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:389
+
+  class DirtyFS : public ThreadsafeFS {
+  public:

Probably needs moving to its own place, but wasn't sure where best to put it.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

sammccall wrote:
> njames93 wrote:
> > sammccall wrote:
> > > doing IO in view() doesn't seem obviously OK.
> > > 
> > > what's the practical consequence of the overlay's inodes not matching 
> > > that of the underlying FS? (It seems reasonable to say that the files 
> > > always have different identity, and may/may not have the same content)
> > It's probably not necessary, but I know preambles use the UniqueID. It may 
> > just be safer to create a uniqueID for each item in the DraftStore and 
> > leave it at that.
> The biggest risks I can see with this approach:
>  - in build-preamble-from-dirty-FS mode, would opening a draft cause the 
> preamble to be considered invalid and needing rebuilding? My read of 
> PrecompiledPreamble::CanReuse is no (UniqueIDs are not stored)
>  - when parsing, if the file can be read through the overlay *and* underlying 
> via different paths (e.g. hardlinks), these won't be deduplicated. However 
> giving them the same inode only changes the nature of the bug: the file would 
> get whichever content was read first.
> 
> So I think it's as good as we can expect.
Unfortunately in the current implementation, opening a draft will invalidate 
and preambles that use the underlying file. 
This is due to the last modification times not matching. 
This could be addressed by querying the TFS on the textDocument/didOpen request 
to get the actual last modification time, but we shouldn't really be doing any 
kind of IO on that thread.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/DraftStore.h:37
 
+  DraftStore(const ThreadsafeFS );
+

sammccall wrote:
> having DraftStore sit on top of TFS seems a bit inside-out, giving it bigger 
> scope than necessary.
> 
> What about giving DraftStore an asVFS() method that returns an in-memory 
> filesystem?
> 
> then separately we can have a separate DirtyFS : TFS, that has a 
> `DraftStore&` and a `TFS ` and implements viewImpl() on top of their 
> public interfaces. Having the dependency in that direction seems more natural 
> to me.
How I had it was a symptom of using the underlying TFS for getting the UniqueID 
of files. But yes now that's gone this approach works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 327646.
njames93 added a comment.

Getting there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DraftStore.cpp
  clang-tools-extra/clangd/DraftStore.h
  clang-tools-extra/clangd/unittests/DraftStoreTests.cpp

Index: clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
===
--- clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
+++ clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
@@ -24,20 +24,20 @@
 
   EXPECT_EQ("25", DS.addDraft(File, "25", ""));
   EXPECT_EQ("25", DS.getDraft(File)->Version);
-  EXPECT_EQ("", DS.getDraft(File)->Contents);
+  EXPECT_EQ("", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("26", DS.addDraft(File, "", "x"));
   EXPECT_EQ("26", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   EXPECT_EQ("27", DS.addDraft(File, "", "x")) << "no-op change";
   EXPECT_EQ("27", DS.getDraft(File)->Version);
-  EXPECT_EQ("x", DS.getDraft(File)->Contents);
+  EXPECT_EQ("x", *DS.getDraft(File)->Contents);
 
   // We allow versions to go backwards.
   EXPECT_EQ("7", DS.addDraft(File, "7", "y"));
   EXPECT_EQ("7", DS.getDraft(File)->Version);
-  EXPECT_EQ("y", DS.getDraft(File)->Contents);
+  EXPECT_EQ("y", *DS.getDraft(File)->Contents);
 }
 
 } // namespace
Index: clang-tools-extra/clangd/DraftStore.h
===
--- clang-tools-extra/clangd/DraftStore.h
+++ clang-tools-extra/clangd/DraftStore.h
@@ -13,6 +13,7 @@
 #include "support/Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include 
 #include 
 #include 
@@ -27,7 +28,7 @@
 class DraftStore {
 public:
   struct Draft {
-std::string Contents;
+std::shared_ptr Contents;
 std::string Version;
   };
 
@@ -47,9 +48,15 @@
   /// Remove the draft from the store.
   void removeDraft(PathRef File);
 
+  llvm::IntrusiveRefCntPtr asVFS() const;
+
 private:
+  struct DraftAndTime {
+Draft Draft;
+time_t MTime;
+  };
   mutable std::mutex Mutex;
-  llvm::StringMap Drafts;
+  llvm::StringMap Drafts;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/DraftStore.cpp
===
--- clang-tools-extra/clangd/DraftStore.cpp
+++ clang-tools-extra/clangd/DraftStore.cpp
@@ -11,6 +11,8 @@
 #include "support/Logger.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -22,7 +24,7 @@
   if (It == Drafts.end())
 return None;
 
-  return It->second;
+  return It->second.Draft;
 }
 
 std::vector DraftStore::getActiveFiles() const {
@@ -75,10 +77,12 @@
  llvm::StringRef Contents) {
   std::lock_guard Lock(Mutex);
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto Res = Drafts.try_emplace(File);
+  auto  = Res.first->getValue();
+  updateVersion(D.Draft, Version);
+  time();
+  D.Draft.Contents = std::make_shared(Contents);
+  return D.Draft.Version;
 }
 
 void DraftStore::removeDraft(PathRef File) {
@@ -87,5 +91,38 @@
   Drafts.erase(File);
 }
 
+namespace {
+
+/// A read only MemoryBuffer shares ownership of a ref counted string. The
+/// shared string object must not be modified while an owned by this buffer.
+class SharedStringBuffer : public llvm::MemoryBuffer {
+  const std::shared_ptr BufferContents;
+  const std::string Name;
+
+public:
+  BufferKind getBufferKind() const override {
+return MemoryBuffer::MemoryBuffer_Malloc;
+  }
+
+  StringRef getBufferIdentifier() const override { return Name; }
+
+  SharedStringBuffer(std::shared_ptr Data, StringRef Name)
+  : BufferContents(std::move(Data)), Name(Name) {
+assert(BufferContents && "Can't create from empty shared_ptr");
+MemoryBuffer::init(BufferContents->c_str(),
+   BufferContents->c_str() + BufferContents->size(), true);
+  }
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr DraftStore::asVFS() const {
+  auto MemFS = llvm::makeIntrusiveRefCnt();
+  std::lock_guard Guard(Mutex);
+  for (const auto  : Drafts)
+MemFS->addFile(Draft.getKey(), Draft.getValue().MTime,
+   std::make_unique(
+   Draft.getValue().Draft.Contents, Draft.getKey()));
+  return MemFS;
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ 

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D94554#2595625 , @njames93 wrote:

> Address some comments, though I have a feeling @Sammccall, I may have to wait 
> until DraftStore has been refactored first before continuing on with this.

D97738  is an attempt at this. It is... 
sprawly, but it's an overdue cleanup that should fit together well with this 
patch, I think.

There may be some conflicts but I don't think it'll be terrible actually... I 
think we should keep going here, it looks really good.




Comment at: clang-tools-extra/clangd/DraftStore.cpp:148
+  static std::unique_ptr
+  create(IntrusiveRefCntPtr Data, StringRef Name) {
+// Allocate space for the FileContentMemBuffer and its name with null

njames93 wrote:
> sammccall wrote:
> > Is there any reason to think we need this micro-optimization? It's quite 
> > invasive.
> Its basically copied from llvm/support/MemoryBuffer.cpp
Sure, but this doesn't really answer the question! Can we keep this trivial 
until we have a reason to optimize it? (Clang typically holds tens of thousands 
of MemoryBuffers, but we're only likely to have a few hundred of these)



Comment at: clang-tools-extra/clangd/DraftStore.cpp:213
+
+class DraftStoreFileSystem : public llvm::vfs::FileSystem {
+public:

njames93 wrote:
> sammccall wrote:
> > can't we use InMemoryFileSystem for this?
> `InMemoryFileSystem` Would work in theory. However using that requires 
> building a lot of unnecessary nodes for thing like intermediate directories.
Probably, but it would keep this code simpler, supports directory iteration and 
status (used in code completion), and I'm fairly confident its path handling is 
solid.

It's certainly possible to add this optimization later if it seems important, 
but in this patch I think it distracts from the design questions.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

njames93 wrote:
> sammccall wrote:
> > doing IO in view() doesn't seem obviously OK.
> > 
> > what's the practical consequence of the overlay's inodes not matching that 
> > of the underlying FS? (It seems reasonable to say that the files always 
> > have different identity, and may/may not have the same content)
> It's probably not necessary, but I know preambles use the UniqueID. It may 
> just be safer to create a uniqueID for each item in the DraftStore and leave 
> it at that.
The biggest risks I can see with this approach:
 - in build-preamble-from-dirty-FS mode, would opening a draft cause the 
preamble to be considered invalid and needing rebuilding? My read of 
PrecompiledPreamble::CanReuse is no (UniqueIDs are not stored)
 - when parsing, if the file can be read through the overlay *and* underlying 
via different paths (e.g. hardlinks), these won't be deduplicated. However 
giving them the same inode only changes the nature of the bug: the file would 
get whichever content was read first.

So I think it's as good as we can expect.



Comment at: clang-tools-extra/clangd/DraftStore.h:37
 
+  DraftStore(const ThreadsafeFS );
+

having DraftStore sit on top of TFS seems a bit inside-out, giving it bigger 
scope than necessary.

What about giving DraftStore an asVFS() method that returns an in-memory 
filesystem?

then separately we can have a separate DirtyFS : TFS, that has a `DraftStore&` 
and a `TFS ` and implements viewImpl() on top of their public interfaces. 
Having the dependency in that direction seems more natural to me.



Comment at: clang-tools-extra/clangd/DraftStore.h:71
 private:
+  struct MetadataDraft {
+Draft Draft;

alternatively you can define whichever internal struct you like, like
```
struct FileData {
  std::string Filename;
  std::string Contents;
  // and MTime and UID
}
```

and store `DenseMap>` (with the keys pointing 
into the values).

You can still hand out `shared_ptr` using the aliasing 
constructor of shared_ptr.

The advantages of this:
 - it avoids the internal representation from being too constrained by the 
public API
 - SharedStringBuffer can easily wrap the whole `shared_ptr`, and so 
you doesn't need a *copy* of the filename

this is a little bit of complexity though, up to you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 327280.
njames93 marked an inline comment as done.
njames93 added a comment.

Address some comments, though I have a feeling @Sammccall, I may have to wait 
until DraftStore has been refactored first before continuing on with this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DraftStore.cpp
  clang-tools-extra/clangd/DraftStore.h
  clang-tools-extra/clangd/unittests/DraftStoreTests.cpp

Index: clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
===
--- clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
+++ clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
@@ -52,8 +52,8 @@
 llvm::Expected Result =
 DS.updateDraft(Path, llvm::None, {Event});
 ASSERT_TRUE(!!Result);
-EXPECT_EQ(Result->Contents, SrcAfter.code());
-EXPECT_EQ(DS.getDraft(Path)->Contents, SrcAfter.code());
+EXPECT_EQ(*Result->Contents, SrcAfter.code());
+EXPECT_EQ(*DS.getDraft(Path)->Contents, SrcAfter.code());
 EXPECT_EQ(Result->Version, static_cast(i));
   }
 }
@@ -84,8 +84,8 @@
   DS.updateDraft(Path, llvm::None, Changes);
 
   ASSERT_TRUE(!!Result) << llvm::toString(Result.takeError());
-  EXPECT_EQ(Result->Contents, FinalSrc.code());
-  EXPECT_EQ(DS.getDraft(Path)->Contents, FinalSrc.code());
+  EXPECT_EQ(*Result->Contents, FinalSrc.code());
+  EXPECT_EQ(*DS.getDraft(Path)->Contents, FinalSrc.code());
   EXPECT_EQ(Result->Version, 1);
 }
 
@@ -345,7 +345,7 @@
 
   Optional Contents = DS.getDraft(File);
   EXPECT_TRUE(Contents);
-  EXPECT_EQ(Contents->Contents, OriginalContents);
+  EXPECT_EQ(*Contents->Contents, OriginalContents);
   EXPECT_EQ(Contents->Version, 0);
 }
 
Index: clang-tools-extra/clangd/DraftStore.h
===
--- clang-tools-extra/clangd/DraftStore.h
+++ clang-tools-extra/clangd/DraftStore.h
@@ -11,8 +11,10 @@
 
 #include "Protocol.h"
 #include "support/Path.h"
+#include "support/ThreadsafeFS.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/FileSystem/UniqueID.h"
 #include 
 #include 
 #include 
@@ -28,10 +30,14 @@
 class DraftStore {
 public:
   struct Draft {
-std::string Contents;
+std::shared_ptr Contents;
 int64_t Version = -1;
   };
 
+  DraftStore(const ThreadsafeFS );
+
+  DraftStore() = default;
+
   /// \return Contents of the stored document.
   /// For untracked files, a llvm::None is returned.
   llvm::Optional getDraft(PathRef File) const;
@@ -59,9 +65,18 @@
   /// Remove the draft from the store.
   void removeDraft(PathRef File);
 
+  const ThreadsafeFS () const;
+
 private:
+  struct MetadataDraft {
+Draft Draft;
+llvm::sys::TimePoint<> MTime;
+llvm::sys::fs::UniqueID UID;
+  };
+  class DraftstoreFS;
+  std::unique_ptr DraftFS;
   mutable std::mutex Mutex;
-  llvm::StringMap Drafts;
+  llvm::StringMap Drafts;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/DraftStore.cpp
===
--- clang-tools-extra/clangd/DraftStore.cpp
+++ clang-tools-extra/clangd/DraftStore.cpp
@@ -9,7 +9,13 @@
 #include "DraftStore.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
+#include "llvm/Support/Chrono.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/FileSystem/UniqueID.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -21,7 +27,7 @@
   if (It == Drafts.end())
 return None;
 
-  return It->second;
+  return It->second.Draft;
 }
 
 std::vector DraftStore::getActiveFiles() const {
@@ -47,14 +53,22 @@
   }
 }
 
+static void updateTime(llvm::sys::TimePoint<> ) {
+  Time = llvm::sys::TimePoint<>::clock::now();
+}
+
 int64_t DraftStore::addDraft(PathRef File, llvm::Optional Version,
  llvm::StringRef Contents) {
   std::lock_guard Lock(Mutex);
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  auto Res = Drafts.try_emplace(File);
+  auto  = Res.first->getValue();
+  if (Res.second)
+D.UID = llvm::vfs::getNextVirtualUniqueID();
+  updateVersion(D.Draft, Version);
+  updateTime(D.MTime);
+  D.Draft.Contents = std::make_shared(Contents);
+  return D.Draft.Version;
 }
 
 llvm::Expected DraftStore::updateDraft(
@@ -68,8 +82,8 @@
  "Trying to do incremental update on non-added document: {0}",
  File);
   }
-  Draft  = EntryIt->second;
-  std::string Contents = EntryIt->second.Contents;
+  MetadataDraft  = EntryIt->second;
+  std::string Contents = *D.Draft.Contents;
 
   for (const TextDocumentContentChangeEvent  : 

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 marked 3 inline comments as done.
njames93 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.h:159
   ClangdServer(const GlobalCompilationDatabase , const ThreadsafeFS ,
-   const Options , Callbacks *Callbacks = nullptr);
+   const ThreadsafeFS , const Options ,
+   Callbacks *Callbacks = nullptr);

sammccall wrote:
> Passing in FS + DirtyFS doesn't seem quite right, because we also pass in all 
> the mutations that create the differences.
> 
> It seem rather that *ClangdServer* should maintain the overlay containing the 
> dirty buffers, and expose it (for use in the few places that ClangdLSPServer 
> currently uses DraftStore).
> 
> (the fact that DraftStore isn't accessible from ClangdServer did also come up 
> when trying to split the *Server classes into Modules, so this seems like a 
> helpful direction from that perspective too)
I was never a fan of this tbh. But moving DraftStore would result in this being 
too big,



Comment at: clang-tools-extra/clangd/DraftStore.cpp:148
+  static std::unique_ptr
+  create(IntrusiveRefCntPtr Data, StringRef Name) {
+// Allocate space for the FileContentMemBuffer and its name with null

sammccall wrote:
> Is there any reason to think we need this micro-optimization? It's quite 
> invasive.
Its basically copied from llvm/support/MemoryBuffer.cpp



Comment at: clang-tools-extra/clangd/DraftStore.cpp:213
+
+class DraftStoreFileSystem : public llvm::vfs::FileSystem {
+public:

sammccall wrote:
> can't we use InMemoryFileSystem for this?
`InMemoryFileSystem` Would work in theory. However using that requires building 
a lot of unnecessary nodes for thing like intermediate directories.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

sammccall wrote:
> doing IO in view() doesn't seem obviously OK.
> 
> what's the practical consequence of the overlay's inodes not matching that of 
> the underlying FS? (It seems reasonable to say that the files always have 
> different identity, and may/may not have the same content)
It's probably not necessary, but I know preambles use the UniqueID. It may just 
be safer to create a uniqueID for each item in the DraftStore and leave it at 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for the long delay here. I've gotten myself buried in too many different 
things.

Overall the idea of exposing drafts as an overlay FS is definitely growing on 
me - it makes it easier and clearer to control which feature is seeing which 
version of the files.

At a high level my main requests are:

- we should move maintenance of the drafts from ClangdLSPServer into 
ClangdServer. (This should maybe have been done a while ago, but this change 
makes it more complicated and therefore pressing)
- we should simplify the implementation as much as possible, and lean on 
existing infrastructure - there's a lot of custom pieces here like RefCntString 
and DraftStoreFileSystem that seem like small optimizations over existing types

(I might take a crack at moving DraftStore by itself, as there probably are 
issues and I want to understand what they are...)




Comment at: clang-tools-extra/clangd/ClangdServer.h:159
   ClangdServer(const GlobalCompilationDatabase , const ThreadsafeFS ,
-   const Options , Callbacks *Callbacks = nullptr);
+   const ThreadsafeFS , const Options ,
+   Callbacks *Callbacks = nullptr);

Passing in FS + DirtyFS doesn't seem quite right, because we also pass in all 
the mutations that create the differences.

It seem rather that *ClangdServer* should maintain the overlay containing the 
dirty buffers, and expose it (for use in the few places that ClangdLSPServer 
currently uses DraftStore).

(the fact that DraftStore isn't accessible from ClangdServer did also come up 
when trying to split the *Server classes into Modules, so this seems like a 
helpful direction from that perspective too)



Comment at: clang-tools-extra/clangd/DraftStore.cpp:145
+
+class RefCntStringMemBuffer : public llvm::MemoryBuffer {
+public:

These classes need comments describing why they exist :-)
(I think you're right that buffers may outlive the VFS)



Comment at: clang-tools-extra/clangd/DraftStore.cpp:148
+  static std::unique_ptr
+  create(IntrusiveRefCntPtr Data, StringRef Name) {
+// Allocate space for the FileContentMemBuffer and its name with null

Is there any reason to think we need this micro-optimization? It's quite 
invasive.



Comment at: clang-tools-extra/clangd/DraftStore.cpp:213
+
+class DraftStoreFileSystem : public llvm::vfs::FileSystem {
+public:

can't we use InMemoryFileSystem for this?



Comment at: clang-tools-extra/clangd/DraftStore.cpp:287
+for (const auto  : DS.Drafts) {
+  // Query the base filesystem for file uniqueids.
+  auto BaseStatus = BaseView->status(KV.getKey());

doing IO in view() doesn't seem obviously OK.

what's the practical consequence of the overlay's inodes not matching that of 
the underlying FS? (It seems reasonable to say that the files always have 
different identity, and may/may not have the same content)



Comment at: clang-tools-extra/clangd/DraftStore.h:25
+/// A Reference counted string that is used to hold the contents of open files.
+class RefCntString : public llvm::ThreadSafeRefCountedBase {
+public:

shared_ptr seems like a cheap alternative here.

However if the point is ultimately to obtain MemoryBuffers that we can hand out 
with the underlying data being refcounted, can we do that directly?

```
class SharedBuffer : public MemoryBuffer {
  std::shared_ptr Data;
public:
  Shared(StringRef Name, std::string Data);
  Shared(const RefcountBuffer &); // copyable
};
```



Comment at: clang-tools-extra/clangd/DraftStore.h:31
+
+  operator const std::string &() const { return Data; }
+  operator StringRef() const { return str(); }

providing non-explicit string conversion operators is IME very likely to cause 
ambiguous overload errors when using this in variaous places :-(



Comment at: clang-tools-extra/clangd/DraftStore.h:82
+  const ThreadsafeFS () const {
+assert(DraftFS && "No draft fs has been set up");
+return *DraftFS;

avoid assert in headers for ODR reasons (ooh, we have a couple of violations i 
should clean up)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94554: [clangd] Add a Filesystem that overlays Dirty files.

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 323378.
njames93 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94554

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DraftStore.cpp
  clang-tools-extra/clangd/DraftStore.h
  clang-tools-extra/clangd/unittests/DraftStoreTests.cpp

Index: clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
===
--- clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
+++ clang-tools-extra/clangd/unittests/DraftStoreTests.cpp
@@ -52,8 +52,8 @@
 llvm::Expected Result =
 DS.updateDraft(Path, llvm::None, {Event});
 ASSERT_TRUE(!!Result);
-EXPECT_EQ(Result->Contents, SrcAfter.code());
-EXPECT_EQ(DS.getDraft(Path)->Contents, SrcAfter.code());
+EXPECT_EQ(*Result->Contents, SrcAfter.code());
+EXPECT_EQ(*DS.getDraft(Path)->Contents, SrcAfter.code());
 EXPECT_EQ(Result->Version, static_cast(i));
   }
 }
@@ -84,8 +84,8 @@
   DS.updateDraft(Path, llvm::None, Changes);
 
   ASSERT_TRUE(!!Result) << llvm::toString(Result.takeError());
-  EXPECT_EQ(Result->Contents, FinalSrc.code());
-  EXPECT_EQ(DS.getDraft(Path)->Contents, FinalSrc.code());
+  EXPECT_EQ(*Result->Contents, FinalSrc.code());
+  EXPECT_EQ(*DS.getDraft(Path)->Contents, FinalSrc.code());
   EXPECT_EQ(Result->Version, 1);
 }
 
@@ -345,7 +345,7 @@
 
   Optional Contents = DS.getDraft(File);
   EXPECT_TRUE(Contents);
-  EXPECT_EQ(Contents->Contents, OriginalContents);
+  EXPECT_EQ(*Contents->Contents, OriginalContents);
   EXPECT_EQ(Contents->Version, 0);
 }
 
Index: clang-tools-extra/clangd/DraftStore.h
===
--- clang-tools-extra/clangd/DraftStore.h
+++ clang-tools-extra/clangd/DraftStore.h
@@ -11,6 +11,7 @@
 
 #include "Protocol.h"
 #include "support/Path.h"
+#include "support/ThreadsafeFS.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
 #include 
@@ -20,6 +21,20 @@
 namespace clang {
 namespace clangd {
 
+/// A Reference counted string that is used to hold the contents of open files.
+class RefCntString : public llvm::ThreadSafeRefCountedBase {
+public:
+  RefCntString(std::string Str) : Data(std::move(Str)) {}
+
+  StringRef str() const { return Data; }
+
+  operator const std::string &() const { return Data; }
+  operator StringRef() const { return str(); }
+
+private:
+  std::string Data;
+};
+
 /// A thread-safe container for files opened in a workspace, addressed by
 /// filenames. The contents are owned by the DraftStore. This class supports
 /// both whole and incremental updates of the documents.
@@ -28,10 +43,14 @@
 class DraftStore {
 public:
   struct Draft {
-std::string Contents;
+llvm::IntrusiveRefCntPtr Contents;
 int64_t Version = -1;
   };
 
+  DraftStore(const ThreadsafeFS );
+
+  DraftStore() = default;
+
   /// \return Contents of the stored document.
   /// For untracked files, a llvm::None is returned.
   llvm::Optional getDraft(PathRef File) const;
@@ -59,9 +78,20 @@
   /// Remove the draft from the store.
   void removeDraft(PathRef File);
 
+  const ThreadsafeFS () const {
+assert(DraftFS && "No draft fs has been set up");
+return *DraftFS;
+  }
+
 private:
+  struct DraftAndTime {
+Draft Draft;
+llvm::sys::TimePoint<> MTime;
+  };
+  class DraftstoreFS;
+  std::unique_ptr DraftFS;
   mutable std::mutex Mutex;
-  llvm::StringMap Drafts;
+  llvm::StringMap Drafts;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/DraftStore.cpp
===
--- clang-tools-extra/clangd/DraftStore.cpp
+++ clang-tools-extra/clangd/DraftStore.cpp
@@ -9,7 +9,9 @@
 #include "DraftStore.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
+#include "llvm/Support/Chrono.h"
 #include "llvm/Support/Errc.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -21,7 +23,7 @@
   if (It == Drafts.end())
 return None;
 
-  return It->second;
+  return It->second.Draft;
 }
 
 std::vector DraftStore::getActiveFiles() const {
@@ -47,14 +49,19 @@
   }
 }
 
+static void updateTime(llvm::sys::TimePoint<> ) {
+  Time = llvm::sys::TimePoint<>::clock::now();
+}
+
 int64_t DraftStore::addDraft(PathRef File, llvm::Optional Version,
  llvm::StringRef Contents) {
   std::lock_guard Lock(Mutex);
 
-  Draft  = Drafts[File];
-  updateVersion(D, Version);
-  D.Contents = Contents.str();
-  return D.Version;
+  DraftAndTime  = Drafts[File];
+  updateVersion(D.Draft, Version);
+  updateTime(D.MTime);
+  D.Draft.Contents = llvm::makeIntrusiveRefCnt(Contents.str());
+  return D.Draft.Version;
 }
 
 llvm::Expected DraftStore::updateDraft(
@@ -68,8 +75,8 @@
  "Trying to do incremental update on non-added