Page MenuHomePhabricator

Move DraftMgr from ClangdServer to ClangdLSPServer
ClosedPublic

Authored by simark on Mar 12 2018, 6:13 PM.

Details

Summary

This patch moves the draft manager closer to the edge of Clangd, from
ClangdServer to ClangdLSPServer. This will make it easier to implement
incremental document sync, by making ClangdServer only deal with
complete documents.

As a result, DraftStore doesn't have to deal with versioning, and thus
its API can be simplified. It is replaced by a StringMap in
ClangdServer holding a current version number for each file.

The current implementation of reparseOpenedFiles is racy. It calls
getActiveDrafts and calls forceReparse for each of them. forceReparse
gets the draft fromt he DraftStore. In theory, it's possible for a file
to have been closed in the mean time. I replaced getActiveDrafts by
forEachActiveDraft. It has the advantage that the DraftStore lock is
held while we iterate on the drafts.

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>

Diff Detail

Repository
rL LLVM

Event Timeline

simark created this revision.Mar 12 2018, 6:13 PM
simark updated this revision to Diff 138253.Mar 13 2018, 1:18 PM

Rebase

Non-trivial rebase on today's master.

ilya-biryukov requested changes to this revision.Mar 14 2018, 5:10 AM
ilya-biryukov added a reviewer: ilya-biryukov.

We shouldn't add Contents parameter to every method, it would defeat the purpose of caching ASTs and won't allow to properly manage lifetimes of the indexes.

The most tricky part is getting rid of DraftMgr in forceReparse. Here's a change that removes usages of it there: D44462`, it should be much easier to make it work when it lands.
Any other reason why we need to add them?

clangd/ClangdLSPServer.h
87 ↗(On Diff #138119)

We can remove this function now, it is equivalent to a one-liner DraftMgr.getDraft(File).Draft

This revision now requires changes to proceed.Mar 14 2018, 5:10 AM

We shouldn't add Contents parameter to every method, it would defeat the purpose of caching ASTs and won't allow to properly manage lifetimes of the indexes.

Makes sense.

The most tricky part is getting rid of DraftMgr in forceReparse. Here's a change that removes usages of it there: D44462`, it should be much easier to make it work when it lands.

I'll take a look, thanks!

Any other reason why we need to add them?

No, just ignorance on my part.

I don't see how to avoid adding the Contents parameter to the codeComplete and signatureHelp methods, since they needed to fetch the document from the draft manager and pass it to the backend.

I don't see how to avoid adding the Contents parameter to the codeComplete and signatureHelp methods, since they needed to fetch the document from the draft manager and pass it to the backend.

You can get the contents inside the runWithPreamble callback:

llvm::Expected<InputsAndPreamble> IP;
auto &Contents = IP->Inputs.Contents;

I don't see how to avoid adding the Contents parameter to the codeComplete and signatureHelp methods, since they needed to fetch the document from the draft manager and pass it to the backend.

You can get the contents inside the runWithPreamble callback:

llvm::Expected<InputsAndPreamble> IP;
auto &Contents = IP->Inputs.Contents;

I see, I made this as a separate patch:

https://reviews.llvm.org/D44484

I see, I made this as a separate patch:

https://reviews.llvm.org/D44484

I LGTMed it, so feel free to submit it.
However, if you do it in this patch it's also fine.

simark updated this revision to Diff 138490.Mar 14 2018, 8:47 PM

Rebase on current master, addressing previous comments

ilya-biryukov added inline comments.Mar 15 2018, 7:38 AM
clangd/ClangdLSPServer.cpp
495 ↗(On Diff #138490)

This function is equivalent to return DraftMgr.getDraft(). Remove and replace all usages with DraftMgr.getDraft()?

clangd/ClangdServer.cpp
154 ↗(On Diff #138490)

Please replace this assert with:

if (!IP)
  return CB(IP.takeError());

(Otherwise the assert will fire when completion is called for non-tracked files, we really want this to be an error instead)

clangd/ClangdServer.h
121 ↗(On Diff #138490)

Let's keep it a StringRef. It means an extra copy for now, DraftMgr return StringRefs instead if we remove mutexes from its interface.

225 ↗(On Diff #138490)

We don't use VersionedContents anywhere. Maybe remove this struct?

236 ↗(On Diff #138490)

This is probably a good place for a small comment:

/// Used to synchronize diagnostic responses for added and removed files.
clangd/DraftStore.h
36 ↗(On Diff #138490)

Let's avoid calling Callback while holding a lock, it is very easy to come up with a code that deadlocks.

I suggest either keeping the getActiveFiles() function or removing the mutex and the locks. IIRC, the DraftStore is never actually used concurrently, so removing mutexes should be no-op change.

simark updated this revision to Diff 138569.Mar 15 2018, 8:39 AM
simark marked 6 inline comments as done.

Changes based on review comments

ilya-biryukov added inline comments.Mar 15 2018, 9:33 AM
clangd/ClangdLSPServer.h
78 ↗(On Diff #138569)

NIT: there is no forceReparse() anymore, maybe remove its mention from the comment?
I.e.

/// Reparses all tracked documents, invalidating compile commands cache.
/// As a result, this method may be very expensive.
/// This method is normally called when the compilation database is changed.
111 ↗(On Diff #138569)

ClangdServer should be the last member! (see the comment above)

clangd/DraftStore.h
48 ↗(On Diff #138569)

No need for Optionals here anymore. Maybe store std::string and erase on removeDraft?

simark updated this revision to Diff 138581.Mar 15 2018, 10:20 AM
simark marked 2 inline comments as done.

Address latest review comments.

Also, update the comments in DraftStore.{h,cpp} that were outdated.

This revision is now accepted and ready to land.Mar 16 2018, 2:36 AM
This revision was automatically updated to reflect the committed changes.