kadircet added a comment. In D97417#2588101 <https://reviews.llvm.org/D97417#2588101>, @qchateau wrote:
> Well indeed do some extra work if we elect a compatible but almost useless > preamble. We'll basically do the work twice (but at least we do it > concurrently \o/). I am afraid this can happen more than twice, e.g. for signature help, the client will issue a request after each keystroke, clangd will see the request try to serve it by patching the AST, while building it we'll receive more requests, and each of those will be served with a huge patching penalty. (as we don't cancel preamble tasks, maybe we should but even then, we'll keep building responses for out-dated requests). > I can look into adding a heuristic layer to detect almost useless preambles > and reject them. Though I'm not sure how I could do this as a single header > can contain 99% of the preamble through include chains That's actually an interesting trade-off we can make. `getBestPreamble` can choose the preamble that has most similar line coverage (this was something i was testing internally already) but in addition to that as you mentioned we can have a threshold and say that we won't consider a preamble as a match, if it requires patching more than X lines(or bytes) of code (note that this heuristic would require doing some IO to calculate number of new lines/bytes, until first preamble is built). Though we would need to ensure that we can still utilize the preamble cache even after that mechanism, and find the sweet spot for X. > Anyway that's the current state of this patch: it's already pretty cool (it > does speed things up noticeably) but there are probably tons of flaws or > potential improvements and I'd like to receive comments from another POV yes it indeed looks great! could you give a bit more information on average preamble build latency in the files you are testing this with, and how all of this works when you open files from different parts of the codebase? (I'll also try to do some analysis for this one myself, especially for the heuristic I mentioned) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97417/new/ https://reviews.llvm.org/D97417 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
