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

Sat Apr 11 08:31:40 PDT 2020

sammccall added a comment.

In D77847#1976020 <https://reviews.llvm.org/D77847#1976020>, @kadircet wrote:

> Thanks, LGTM!
>
> In D77847#1975940 <https://reviews.llvm.org/D77847#1975940>, @sammccall wrote:
>
> > In D77847#1974126 <https://reviews.llvm.org/D77847#1974126>, @kadircet wrote:
> >
> > > should we also have a unittest for checking ast caching works as expected?
> >
> >
> > TUSchedulerTests has `NoopOnEmptyChanges` which I think tests exactly this, and `EvictedAST` which checks it's sensitive to the cache.
>
>
> Right, but none of them checks exactly this behaviour.  The `NoopOnEmptyChanges` test only ensures we don't publish diagnostics again
>  and `EvictedAST` test actually asserts on the opposite, it checks the last updated file is "always" cached. Maybe extending `EvictedAST`
>  test with a "noop update" might be a better fit. Up to you though.

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. 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.

(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)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77847