This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Config: loading and caching config from disk.
ClosedPublic

Authored by sammccall on Jul 1 2020, 7:43 AM.

Details

Summary

The Provider extension point is designed to also be implemented by
ClangdLSPServer (to inject config-over-lsp) and likely by embedders.

Diff Detail

Event Timeline

sammccall created this revision.Jul 1 2020, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 7:43 AM
njames93 added inline comments.
clang-tools-extra/clangd/ConfigProvider.h
59

a fixed path

thanks looks really cool!

clang-tools-extra/clangd/ConfigProvider.cpp
40

I believe this function is going to be hot, I would suggest putting a span here to track the latencies

42

i suppose doing this IO on most of our features are OK, as they tend to search for .clang-format at least (even the ancestor traversal), but I wonder whether these will also be triggered for code completion and signature help, as they currently don't do any IO (apart from preamble deserialization, and maybe one patched include file).

Not a concern for this patch, but something to keep in mind when integrating with the rest.

58

is this really necessary ? we are setting the cache key to old stat values anyway, so the next read shouldn't match neither the Size nor MTime.

68

nit: I've found the nestedness a bit overwhelming, wdyt about putting this into a scope_exit function and using early returns above ?

121

ummm, what ? is this a bug in path iterator that should be fixed rather than worked around?

182

nit: I think this looks a bit ... surprising

clang-tools-extra/clangd/ConfigProvider.h
72

maybe also mention precedence order ? I suppose it already has the natural, the latter one will override the former behavior, but saying out explicitly wouldn't hurt.

74

maybe chain instead of combine ?

85

first part of this sentence doesn't say much, maybe drop it?

91

doesn't seem to be implemented anywhere? I suppose it is just a convenience wrapper around Provider::fromYAMLFile, is it worth it?

sammccall marked 15 inline comments as done.Jul 2 2020, 9:25 AM
sammccall added inline comments.
clang-tools-extra/clangd/ConfigProvider.cpp
40

Tracing isn't trivially cheap: that usually involves taking a mutex, writing to a buffer, and I think flushing it to disk.

So we shouldn't trace things that are both hot and small, which I think this is.

I added a tracer to Provider::getConfig - OK to revisit if we need something more fine-grained?

42

Yeah, this is a good point.
One possible design would be to have a "latency-sensitivity" setting, probably on Params, and not bothering to stat the file if we're highly sensitive. (i.e. always reuse the cached value unless the cache is uninitialized).

I've added this idea to the class comment.

Related: I think an abstraction like this, with some thought put into invalidation policy, should be eventually used for .clang-format, .clang-tidy, compile_commands.json, as we should be able to get fresh results with a fairly tiny amount of IO.

58

I considered doing nothing here, or setting Size = Buf->size.

But there are *possible* scenarios where you go back to the old state (think switching git branches) and then end up with a poisoned cache.

And this is cheap (simple, and also doesn't actually cause you to miss the cache when you want to hit it)

68

scope_exit is a neat idea, but when I tried it I found the code hard to reason about.

I extracted a function instead.

Unnesting this caused me to catch a case i'd missed: if stat succeeds but read fails (once), I'd prefer to retry the read before caching the failure forever... (again, the branch switching workflows)

121

No, it's documented that Component isn't necessarily part of the path.
The only case I could find of this actually happening is that "/foo/bar/" iterates as "foo", "bar", ".", at least in some cases.

Extended the comment a bit.

182

the fact that this is Fragment instead of Fragment(P, C)?

Yeah, this is an argument for adding an interface rather than using std::function.
But I don't think it's a big deal or really actionable in this patch.

clang-tools-extra/clangd/ConfigProvider.h
85

I think just saying "this function must be threadsafe" is a bit confusing because it's a const method, it's like saying "this function does not throw exceptions" in llvm code.

But I shoud have made my point more explicit. Replaced with "Despite the need for caching..."

91

Whoops, this was copy/pasted from a prototype.
I decided Provider::fromYAMLFile was a better name.

sammccall updated this revision to Diff 275146.Jul 2 2020, 9:26 AM
sammccall marked 5 inline comments as done.

address comments

sammccall updated this revision to Diff 275165.Jul 2 2020, 10:49 AM

Try to get the tests to pass on windows.

kadircet added inline comments.Jul 3 2020, 2:44 AM
clang-tools-extra/clangd/ConfigProvider.cpp
40

why do we want to cache failure case for missing files ?

If it is truly missing and we didn't race with another process (likely git-checkout), the next stat will fail and we'll return a no-op fragment.
If it was a race, we want to pick it up on the next run (and reading twice might not have enough of a delay in between), so why not just cache with a sentinel key in here too?

sammccall updated this revision to Diff 275384.Jul 3 2020, 6:57 AM
sammccall marked 2 inline comments as done.

Don't cache failed opens.

clang-tools-extra/clangd/ConfigProvider.cpp
40

Yeah I was worried about a file we can stat but chronically can't read (e.g. because permissions are wrong). But this is probably not worth worrying about. Changed no not cache as you suggest.

kadircet accepted this revision.Jul 3 2020, 12:22 PM

thanks LGTM!

This revision is now accepted and ready to land.Jul 3 2020, 12:22 PM
This revision was automatically updated to reflect the committed changes.