Use byte offsets for internal ranges#128
Conversation
6ec43a3 to
dbe2165
Compare
There was a problem hiding this comment.
The change looks pretty good to me, and makes sense.
In practice, I assume only the workspace symbol and document symbol providers might notably be affected performance-wise. In critical cases we always can modify the concrete ...Impl struct generation to mitigate potential runtime performance issues - if such ever show up, right?
I have just some API-related hints/questions.
dbe2165 to
01e9344
Compare
01e9344 to
385d8a8
Compare
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: b69a4a6 | Previous: d803417 | Ratio |
|---|---|---|---|
BenchmarkWorkspaceCycle (typefox.dev/fastbelt/examples/statemachine) - MB/s |
12.98 MB/s |
5.21 MB/s |
2.49 |
This comment was automatically generated by workflow using github-action-benchmark.
sailingKieler
left a comment
There was a problem hiding this comment.
This looks really nice, many source portions feel much more streamlined.
I have just two very minor points, see below. 🚀
To reduce on memory usage, this change removes the line/column information for all internal text ranges and only leaves the byte-offset ranges in place. On one hand, this simplifies some code, while making some other more complicated (especially LSP response handling).
Reduces the memory usage of tokens by 20% (16 bytes). Also removes 16 bytes from all AST nodes. Since we no longer have to track utf16 line/column infos, this speeds up the lexer by roughly 10%.