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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D94554#2595625 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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<RefCntStringMemBuffer>
+  create(IntrusiveRefCntPtr<RefCntString> 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 &KV : 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 &BaseFS);
+
----------------
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 &Base` 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<StringRef, shared_ptr<FileData>>` (with the keys pointing into the values).

You can still hand out `shared_ptr<const std::string>` 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<FileData>`, 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