Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(js_analyze): use Box<str>/Box<[_]> to reduce memory usage #4211

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Oct 8, 2024

Summary

This PR replace Vec<_> and String with their boxed versions Box<[_]> and Box<str> in Rules' Options and Signals. This allows reducing memory usage and could improve perf in some scenario.

To do so I implemented Deserializable for Box<[_]> and Box<str>.

Also, I found some use of text instead of trimmed_text.
I took the opportunity of replacing text with trimmed_text.

Test Plan

CI must pass.

@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Oct 8, 2024
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48584 48584 0
Passed 47393 47393 0
Failed 1191 1191 0
Panics 0 0 0
Coverage 97.55% 97.55% 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 6576 6576 0
Passed 2204 2204 0
Failed 4372 4372 0
Panics 0 0 0
Coverage 33.52% 33.52% 0.00%

ts/babel

Test result main count This PR count Difference
Total 680 680 0
Passed 608 608 0
Failed 72 72 0
Panics 0 0 0
Coverage 89.41% 89.41% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18474 18474 0
Passed 14147 14147 0
Failed 4327 4327 0
Panics 0 0 0
Coverage 76.58% 76.58% 0.00%

Copy link

codspeed-hq bot commented Oct 8, 2024

CodSpeed Performance Report

Merging #4211 will improve performances by 9.25%

Comparing conaclos/box-str-slice (f8941a1) with main (f8d9173)

Summary

⚡ 1 improvements
✅ 104 untouched benchmarks

Benchmarks breakdown

Benchmark main conaclos/box-str-slice Change
big5-added_15586211152145260264.json[cached] 473.5 µs 433.5 µs +9.25%

@Conaclos Conaclos force-pushed the conaclos/box-str-slice branch from a08b0bf to aa7b4a3 Compare October 8, 2024 13:36
@ematipico
Copy link
Member

@Conaclos did you see concrete differences by any chance when running the CLI?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Since you changed many lint rules, you might as well want to update the contribution guide of the analyzer, and add a chapter that talks about it.

@@ -4132,7 +4132,6 @@
"properties": {
"attributes": {
"description": "Additional attributes that will be sorted.",
"default": ["class", "className"],
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This avoids some allocations and doesn't change the behavior of the rule.

@Conaclos
Copy link
Member Author

Conaclos commented Oct 8, 2024

did you see concrete differences by any chance when running the CLI?

Unfortunately it is not significant in the test I did. However, this reduces a bit the size of our configuration. This is always good to take.

Since you changed many lint rules, you might as well want to update the contribution guide of the analyzer, and add a chapter that talks about it.

I added some notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants