Skip to content

Conversation

@ematipico
Copy link
Member

Summary

Address #5990

I'm not sure this PR will fix ALL the performance bottlenecks of the batch mutations. Unfortunately, there is one point inside batch.rs where we're kinda forced to allocate a new strings because we need to print an entire node with its trivia, and that's also recursive. I will have to find a smart way to emit &str when printing a node. I will leave a comment to show the line.

I also reduced the use of to_strimmed_string around lint rules. What I find saddening is that we often try to print nodes instead of extracting the token we want to check. Maybe we should have some guide that explains how to do so.

Test Plan

Current tests should still pass

@github-actions github-actions bot added A-Core Area: core A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS labels May 13, 2025
Comment on lines +511 to +514
SyntaxElement::Node(node) => {
text_edit_builder
.with_unicode_words_diff(old, &node.to_string());
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we're diffing a node, and unfortunately we need to extract the entire string: tokens, trivia and child nodes/tokens

@github-actions
Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 50524 50524 0
Passed 49209 49209 0
Failed 1315 1315 0
Panics 0 0 0
Coverage 97.40% 97.40% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6648 6648 0
Passed 2229 2229 0
Failed 4419 4419 0
Panics 0 0 0
Coverage 33.53% 33.53% 0.00%

ts/babel

Test result main count This PR count Difference
Total 807 807 0
Passed 718 718 0
Failed 89 89 0
Panics 0 0 0
Coverage 88.97% 88.97% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18702 18702 0
Passed 14349 14349 0
Failed 4353 4353 0
Panics 0 0 0
Coverage 76.72% 76.72% 0.00%

@ematipico ematipico requested review from a team May 13, 2025 09:27
@codspeed-hq
Copy link

codspeed-hq bot commented May 13, 2025

CodSpeed Performance Report

Merging #5997 will not alter performance

Comparing perf/reduce-use-of-strings (d84b0ff) with main (73be9e1)

Summary

✅ 95 untouched benchmarks

@ematipico ematipico merged commit 72f8e94 into main May 14, 2025
12 of 13 checks passed
@ematipico ematipico deleted the perf/reduce-use-of-strings branch May 14, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Core Area: core A-Linter Area: linter A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants