Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

perf(rome_analyze): optimize code actions diff generation #2842

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Jul 7, 2022

Summary

The generation of code action diffs shows up a lot when profiling the analyzer, so I've optimized it a bit by making it skip over irrelevant subtrees. For find_diff_range this implies comparing the green nodes between the old and new version of the tree, so I've made the key method public on SyntaxNode / SyntaxToken / SyntaxElement

Test Plan

The snapshot tests for the analyzer still pass, as well as the unit tests for find_diff_range although I had to change the expected values since the function now compares the green nodes instead of the text content

@leops leops temporarily deployed to aws July 7, 2022 13:09 Inactive
@github-actions
Copy link

github-actions bot commented Jul 7, 2022

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 388 388 0
Failed 5558 5558 0
Panics 0 0 0
Coverage 6.53% 6.53% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12393 12393 0
Failed 3864 3864 0
Panics 0 0 0
Coverage 76.23% 76.23% 0.00%

@ematipico
Copy link
Contributor

!bench_analyzer

@ematipico
Copy link
Contributor

ematipico commented Jul 8, 2022

@leops
Copy link
Contributor Author

leops commented Jul 8, 2022

The action died: https://github.com/rome/tools/runs/7249709125?check_suite_focus=true

That's probably due to a timeout, until #2847 is merged the current benchmark suite for the analyzer is pretty much useless

@IWANABETHATGUY
Copy link
Contributor

I was wondering any reason why we don't use a string-based diff algorithm?

@leops
Copy link
Contributor Author

leops commented Jul 11, 2022

I was wondering any reason why we don't use a string-based diff algorithm?

This is technically all about converting a CST-based diff into a text-based diff, but for the initial version the diff only consist of a single range-replace operation since it was easier to just implement that first. We do have the capability to eventually represent more complex string diffs with the rome_text_edit crate, and the switch could happen sooner rather than later if the theoretically simpler algorithm we have now continues to require some tweaking to run correctly

@leops leops merged commit 58d7ee4 into main Jul 11, 2022
@leops leops deleted the refactor/find-diff-range branch July 11, 2022 13:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants