This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Move DirBasedCDB broadcasting onto its own thread.
ClosedPublic

Authored by sammccall on Jan 13 2021, 8:13 AM.

Details

Summary

This is on the critical path (it blocks getting the compile command for
the first file).

It's not trivially fast: it involves processing all filenames in the CDB
and doing some IO to look for shadowing CDBs.

And we may make this slower soon - making CDB configurable implies evaluating
the config for each listed to see which ones really are owned by the
broadcasted CDB.

Diff Detail

Event Timeline

sammccall created this revision.Jan 13 2021, 8:13 AM
sammccall requested review of this revision.Jan 13 2021, 8:13 AM
kadircet added inline comments.Jan 15 2021, 3:47 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
496

nit: we already hold the lock within the loop while reading. it is nice that we no longer need to acquire the lock during destructor, but is it worth the extra mental load that comes with memory orderings? (also IIRC, default load/store behaviors are already acquire-release)

541

don't we need some context propagation here? previously broadcasting was running with the context of the astworker that discovered the cdb. i suppose it is not needed at the moment, as most of the stuff we do in ::process isn't context-aware, but I can't be sure if we don't have any users that make use of some context-aware FS.

sammccall updated this revision to Diff 317365.Jan 18 2021, 8:07 AM
sammccall marked an inline comment as done.

address comments

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
496

Whoops, I split this patch in half and now this makes no sense.

The point is rather that we should interrupt the "evaluate the config for every file" loop (which will be part of process()) if ShouldStop is set. So we check this atomic inside that loop, without acquiring the lock.

(In practice maybe this is always "fast enough" that we should just run the whole loop, enqueue all the background indexing stuff, and then shut down afterwards?)

(also IIRC, default load/store behaviors are already acquire-release)

Default is sequentially consistent, which is almost always more than we need.


I've added a check to the existing loop (which probes directory caches), less necessary but shows the idea. If you prefer to drop this entirely that's OK too.

kadircet accepted this revision.Jan 19 2021, 4:08 AM

thanks, lgtm!

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
496

The point is rather that we should interrupt the "evaluate the config for every file" loop (which will be part of process()) if ShouldStop is set. So we check this atomic inside that loop, without acquiring the lock.

ah okay now it makes sense.

I've added a check to the existing loop (which probes directory caches), less necessary but shows the idea. If you prefer to drop this entirely that's OK too.

i suppose we could end up blocking the shutdown for quite some time in pathological cases due to disk io. so this looks like a good tradeoff.

This revision is now accepted and ready to land.Jan 19 2021, 4:08 AM
This revision was landed with ongoing or failed builds.Jan 20 2021, 2:23 AM
This revision was automatically updated to reflect the committed changes.