This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Minimal implementation of automatic static index, behind a flag.
ClosedPublic

Authored by sammccall on Oct 9 2018, 11:51 AM.

Details

Summary

See tinyurl.com/clangd-automatic-index for design and goals.

Lots of limitations to keep this patch smallish, TODOs everywhere:

  • no serialization to disk
  • no changes to dynamic index, which now has a much simpler job
  • no partitioning of symbols by file to avoid duplication of header symbols
  • no reindexing of edited files
  • only a single worker thread
  • compilation database is slurped synchronously (doesn't scale)
  • uses memindex, rebuilds after every file (should be dex, periodically)

Still needs tests, but should be ready for review of the basic shape.

Event Timeline

sammccall created this revision.Oct 9 2018, 11:51 AM

Looks good in general.

clangd/ClangdLSPServer.h
117 ↗(On Diff #168847)

please document what the callback is for and how often it's called.

clangd/index/Background.cpp
64

Maybe add a trace?

99

Why is this needed?

164

Add a FIXME to use Dex?

clangd/index/Background.h
2

Is it possible to add test for this?

29

s/TODO/FIXME/?

40

Maybe add a FIXME for files not in CDB e.g. headers?

clangd/tool/ClangdMain.cpp
170 ↗(On Diff #168847)

Should we ignore index-file/auto-index if one is set?

sammccall marked 6 inline comments as done.Oct 12 2018, 7:45 AM
sammccall added inline comments.
clangd/index/Background.cpp
99

Removed. I copied it from the related code in JSONCompilationDatabase, but I'm not sure what the right behavior is, as JSONCompilationDatabase doesn't specify the format of paths.

164

Added it to the existing TODO. They're coupled: dex is expensive to build, so rebuilding it after every file wouldnt' make sense.

clangd/index/Background.h
2

Added a test that indexes a file, waits for the queue to be empty, and queries.

That's pretty basic, do you have suggestions for things you'd like to see tested?

40

There's nothing missing for headers at this point.
Headers will already be indexed (as part of any TU that includes them).
Later, we'll partition the outputs by file, but these headers will still be indexed.
When it comes time to watch files on disk for changes, then we'll need to consider headers specially.

Indexing headers that are not included by any file is out of scope (it's not clear this is a good idea, e.g. consider files under test/ in llvm).

clangd/tool/ClangdMain.cpp
170 ↗(On Diff #168847)

Since this is hidden/experimental, I think it's not worth adding explicit code so they interact nicely, I just documented the behavior instead instead.

sammccall updated this revision to Diff 169399.Oct 12 2018, 7:46 AM
sammccall marked 4 inline comments as done.

Address comments, add tests.
Some changes (enqueue(), blockUntilIdleForTest()) to support testing.

sammccall added inline comments.Oct 12 2018, 7:50 AM
clangd/ClangdLSPServer.h
117 ↗(On Diff #168847)

Documented at the callsite and in GlobalCompilationDatabase.h.
This class here is really just plumbing (it shouldn't be in the header at all)

sammccall updated this revision to Diff 169400.Oct 12 2018, 7:50 AM

Oops, forgot one comment.

ioeric accepted this revision.Oct 12 2018, 8:17 AM
ioeric added inline comments.
clangd/index/Background.cpp
52

Maybe generalize this to NumActiveTasks? Currently, this is single-threaded and safe, but it could be missed when we add more threads.

unittests/clangd/BackgroundIndexTests.cpp
15

Also add a test for enqueueAll with multiple TUs ?

This revision is now accepted and ready to land.Oct 12 2018, 8:17 AM
sammccall added inline comments.Oct 12 2018, 8:21 AM
unittests/clangd/BackgroundIndexTests.cpp
15

Is it important to call enqueueAll specifically vs enqueue multiple times?

We don't have a good test fixture for a compilation database, and enqueueAll is trivial...

ioeric added inline comments.Oct 12 2018, 8:28 AM
unittests/clangd/BackgroundIndexTests.cpp
15

I think the randomization code worths a test.

How about adding a test in ClangdServer with the auto index enabled? I think we'd also want coverage in ClangdServer anyway.

sammccall added inline comments.Oct 12 2018, 8:39 AM
unittests/clangd/BackgroundIndexTests.cpp
15

How would you suggest testing the randomization :-)

The problem with a ClangdServer test is that it doesn't know anything about autoindex.
AutoIndex lives in ClangdLSPServer, because that's where the compilation database lives (because people keep cramming LSP extensions in to manipulate it, and I haven't been able to remove them).
Nobody has worked out how to test ClangdLSPServer yet. It's a worthy project, but...

ioeric added inline comments.Oct 12 2018, 9:03 AM
unittests/clangd/BackgroundIndexTests.cpp
15

How would you suggest testing the randomization :-)

Not suggesting testing the behavior; just test that it works really. I guess a single file would also do it.

The problem with a ClangdServer test is that it doesn't know anything about autoindex.

Ah, I see. That's a bit unfortunate. ClangdServer seems to be a better place for the auto index. I wonder if we could move AutoIndex into ClangdServer? I'm not very familiar with CDB APIs, but intuitively this seems doable if we tweak the interface of CDB a bit e.g. inject the callback into GlobalCompilationDatabase without going through CompilationDB? Not sure if how well this would play with the CDB design though.

sammccall added inline comments.Oct 12 2018, 9:52 AM
unittests/clangd/BackgroundIndexTests.cpp
15

ClangdServer seems to be a better place for the auto index

I fully agree.

I wonder if we could move AutoIndex into ClangdServer? I'm not very familiar with CDB APIs...

Currently the global CDB is trying to be all things to all people:

  • by default, it follows Tooling conventions and looks for compile_commands above the source files
  • google's hosted option wants it to be completely opaque, no indexing
  • apple wants it to be an in-memory store mutated over LSP
  • ericsson wants it to be a fixed directory set by the user

And we've implemented these in ad-hoc ways, not behind any real abstraction. I don't think adding more functionality to the interface without addressing this is a great idea, and I don't really know how to untangle all this without asking people to rethink their use cases. But I'll think about this some more.

sammccall updated this revision to Diff 169694.Oct 15 2018, 6:29 AM
sammccall marked an inline comment as done.

Address comments, strip out of clangdlspserver for now.

sammccall added inline comments.Oct 15 2018, 6:30 AM
unittests/clangd/BackgroundIndexTests.cpp
15

After thinking about this, decided to just check in the library with unit tests for now while we sort out the layering.
I've ripped out all the ClangdMain and ClangdLSPServer changes.

ioeric accepted this revision.Oct 15 2018, 6:33 AM

still lgtm

clangd/Compiler.cpp
83

Maybe also revert this change?

This revision was automatically updated to reflect the committed changes.