Accidentally quadratic

[matklad]

Our syntax highlighting is accdentally quadratic. Current state of the PR fixes it in a pretty crude way, looks like for the proper fix we need to redo how source-analyzer works.

NB: don't be scared by diff stats, that's mostly a test-data file

[matklad]

@flodiebold curious what you think about this!

The problem here is that, when we go from InFile<AstNode> to hir::SomeID, we construct a DynMap, which stores SyntaxNode -> ID essentially. Construction the map is linear, and we did it for every child. (which goes back to your question about throwing away the map after we've checked a single entry).

Now, the obvious solution here is to add caching, however, it turns out not to be too straight-forward to implement. At least, not straight forward with salsa.

• the first problem is that we want to store syntax trees, and I am afraid of caching syntax trees in salsa. It looks like with salsa garbage is generally collected in-between modifications (with the exception of LRU), and so we can accamulate a lot of stuff. I think the right approach here is to do caching on per-lsp request basis. IE, caching for highlighting of a single file
• the second problem is that we want to store syntax trees, and SyntaxNodes in particular. And SyntaxNodes are not thread safe, so we can't put them in salsa (and yeas, Rust saved me couple of days of debugging, because I absolutely tried to do just that :-) ). We can use the usual trick of using a ptr instead of a node, but that leads to a more awkward API I think.

So, instead we introduce a SourceBinder, which holds a light-weight cache on top of the salsa. I think SourceBinder could potentially become a single entry point for IDE API. It is somewhat similar to exisrting SourceAnalyzer, but:

• it can be reused freely in any context (while SourceAnalyzer is only valid in the context where it was created, and the precise definition of what that exactly means does not exist)
• it also does what FromSource did before.

[flodiebold]

Hmm, I have to think about this a bit, but this:

SourceAnalyzer is only valid in the context where it was created, and the precise definition of what that exactly means does not exist

is something that I noticed a lot when working on the assists. It made the API feel wrong, because it doesn't help you at all to use the correct SourceAnalyzer...

[kjeremy]

As an aside... are there operations we should be continuously benchmarking?

[matklad]

@kjeremy I don't think continuous benchmarking would be too valuable at the moment, as we haven't settled down on the overall architecture I think. That is, I expect that various random refactorings would affect benchmarks more than targeted optimization work.

We should have tests that something isn't exponentially slow though, like the one added in this PR, whose failure criteria is basically "the test suite never completes".

[matklad]

Also, it probably makes sense to fix things like #2821 (comment) or rust-lang/chalk#298 before starting the benchmarks.

[kjeremy]

typo?

[flodiebold]

Makes sense to me!

[matklad]

bors r+

[bors]

Build succeeded

• Rust (macos-latest)
• Rust (ubuntu-latest)
• TypeScript