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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
