[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D77847#1976077 <https://reviews.llvm.org/D77847#1976077>, @sammccall wrote:

> OK, I misunderstood what we're testing, please tell me if I got it right this time :-)
>
> Plausible bad behavior: we send a no-op change to a relatively inactive file. TUScheduler builds an AST (wasting CPU) and caches it (wasting CPU + latency by displacing a useful entry).
>
> We want to prevent either of those from happening. Caching is indirectly observable, building an AST isn't particularly observable.


Building an AST is also observable(at least in the absence of read requests), as we always generate diagnostics afterwards and that's what `TUSchedulerTests.NoopOnEmptyChange` actually asserts(that we don't build ASTs on noop changes).
I was suggesting to test the "cache" behaviour separately from building an ast. I know that's redundant today, as we never cache an ast if we haven't build any, but there are no tests ensuring it stays that way.

> If building an AST *was* observable, then we have an alternate (IMO more meaningful) way of measuring the cache effects: try to use read the maybe-evicted AST and see if it rebuilds.
>  So I think it would be nicer to structure a test around rebuilds.
> 
> A couple of options I can think of:
> 
> - add an API like `unsigned TUScheduler::buildCount(PathRef)`, exposing a counter only used for testing? (Or extend the memory usage API to provide this info too)
> - register a tracer for the test, and count `BuildAST` events (not too fragile if we assert there are exactly N > 0 of them). This seems hacky, technique/tracer may be reusable for other tests. Not sure whether that's a good thing.

I suppose that could be a more robust way of checking if an AST was build rather than waiting for diags to be released. I believe the former would be enough(preferably just extending the existing API to not create more test-only endpoints).

> (Unless you object, I'll land this patch once I have a test out for review so we're sure the existing behavior this patch depends on is there)

I totally agree, please do so.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77847/new/

https://reviews.llvm.org/D77847