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