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

feat(rome_js_formatter): remove unnecessary escapes #2804

Merged
merged 4 commits into from
Jul 4, 2022

Conversation

ematipico
Copy link
Contributor

Summary

This PR closes #2444

I changed the iteration over the string using chars(). This helped to make some edge case easier to handle. Also, char provides some built-in methods that helped to cover the majority of the cases.

Test Plan

cargo test and check manually that the changes matches prettier's snapshots

PR
File Based Average Prettier Similarity: 77.26%
Line Based Average Prettier Similarity: 72.48%

main
File Based Average Prettier Similarity: 77.25%
Line Based Average Prettier Similarity: 72.45%

@ematipico ematipico temporarily deployed to aws July 1, 2022 10:38 Inactive
@ematipico ematipico requested review from leops and yassere July 1, 2022 10:38
@ematipico ematipico force-pushed the feature/remove-unnecessary-escape branch from bbd7788 to dee4b89 Compare July 1, 2022 10:39
@cloudflare-pages
Copy link

cloudflare-pages bot commented Jul 1, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c2f60f0
Status: ✅  Deploy successful!
Preview URL: https://9d40dd07.tools-8rn.pages.dev
Branch Preview URL: https://feature-remove-unnecessary-e.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws July 1, 2022 10:39 Inactive
@ematipico ematipico temporarily deployed to aws July 1, 2022 10:42 Inactive
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

@ematipico
Copy link
Contributor Author

!bench_formatter

@ematipico ematipico requested a review from leops July 1, 2022 13:16
@ematipico ematipico temporarily deployed to aws July 1, 2022 13:16 Inactive
@ematipico
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    375.3±3.36ms     6.9 MB/sec    1.03    386.8±4.78ms     6.7 MB/sec
formatter/compiler.js                    1.00    237.7±0.69ms     4.4 MB/sec    1.02    242.8±0.66ms     4.3 MB/sec
formatter/d3.min.js                      1.00    204.6±0.90ms  1311.7 KB/sec    1.02    208.3±1.58ms  1288.7 KB/sec
formatter/dojo.js                        1.00     13.5±0.09ms     5.1 MB/sec    1.02     13.8±0.05ms     5.0 MB/sec
formatter/ios.d.ts                       1.00    252.1±1.61ms     7.4 MB/sec    1.04    261.3±1.62ms     7.1 MB/sec
formatter/jquery.min.js                  1.00     52.1±0.18ms  1625.2 KB/sec    1.02     53.0±0.20ms  1595.9 KB/sec
formatter/math.js                        1.00    384.9±1.03ms  1722.6 KB/sec    1.04    400.1±2.44ms  1657.2 KB/sec
formatter/parser.ts                      1.00      8.7±0.03ms     5.5 MB/sec    1.02      8.9±0.05ms     5.4 MB/sec
formatter/pixi.min.js                    1.00    221.2±0.64ms  2031.4 KB/sec    1.02    226.3±1.43ms  1986.0 KB/sec
formatter/react-dom.production.min.js    1.00     65.2±0.32ms  1808.4 KB/sec    1.03     67.3±0.33ms  1752.4 KB/sec
formatter/react.production.min.js        1.00      3.2±0.02ms  1959.7 KB/sec    1.02      3.3±0.02ms  1929.3 KB/sec
formatter/router.ts                      1.00      6.5±0.01ms     9.1 MB/sec    1.02      6.6±0.03ms     8.9 MB/sec
formatter/tex-chtml-full.js              1.00    503.4±3.03ms  1853.9 KB/sec    1.03    518.7±3.93ms  1798.8 KB/sec
formatter/three.min.js                   1.00    253.9±0.95ms     2.3 MB/sec    1.03    260.6±0.69ms     2.3 MB/sec
formatter/typescript.js                  1.00   1557.4±8.32ms     6.1 MB/sec    1.03   1601.1±9.36ms     5.9 MB/sec
formatter/vue.global.prod.js             1.00     84.5±1.02ms  1459.5 KB/sec    1.02     86.2±0.50ms  1430.6 KB/sec

@ematipico
Copy link
Contributor Author

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    375.3±3.36ms     6.9 MB/sec    1.03    386.8±4.78ms     6.7 MB/sec
formatter/compiler.js                    1.00    237.7±0.69ms     4.4 MB/sec    1.02    242.8±0.66ms     4.3 MB/sec
formatter/d3.min.js                      1.00    204.6±0.90ms  1311.7 KB/sec    1.02    208.3±1.58ms  1288.7 KB/sec
formatter/dojo.js                        1.00     13.5±0.09ms     5.1 MB/sec    1.02     13.8±0.05ms     5.0 MB/sec
formatter/ios.d.ts                       1.00    252.1±1.61ms     7.4 MB/sec    1.04    261.3±1.62ms     7.1 MB/sec
formatter/jquery.min.js                  1.00     52.1±0.18ms  1625.2 KB/sec    1.02     53.0±0.20ms  1595.9 KB/sec
formatter/math.js                        1.00    384.9±1.03ms  1722.6 KB/sec    1.04    400.1±2.44ms  1657.2 KB/sec
formatter/parser.ts                      1.00      8.7±0.03ms     5.5 MB/sec    1.02      8.9±0.05ms     5.4 MB/sec
formatter/pixi.min.js                    1.00    221.2±0.64ms  2031.4 KB/sec    1.02    226.3±1.43ms  1986.0 KB/sec
formatter/react-dom.production.min.js    1.00     65.2±0.32ms  1808.4 KB/sec    1.03     67.3±0.33ms  1752.4 KB/sec
formatter/react.production.min.js        1.00      3.2±0.02ms  1959.7 KB/sec    1.02      3.3±0.02ms  1929.3 KB/sec
formatter/router.ts                      1.00      6.5±0.01ms     9.1 MB/sec    1.02      6.6±0.03ms     8.9 MB/sec
formatter/tex-chtml-full.js              1.00    503.4±3.03ms  1853.9 KB/sec    1.03    518.7±3.93ms  1798.8 KB/sec
formatter/three.min.js                   1.00    253.9±0.95ms     2.3 MB/sec    1.03    260.6±0.69ms     2.3 MB/sec
formatter/typescript.js                  1.00   1557.4±8.32ms     6.1 MB/sec    1.03   1601.1±9.36ms     5.9 MB/sec
formatter/vue.global.prod.js             1.00     84.5±1.02ms  1459.5 KB/sec    1.02     86.2±0.50ms  1430.6 KB/sec

I guess this was expected, but I think it's a price that we can pay for having better checks and and control over the strings. What do you guys think?

Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

I guess one important optimization we could do is to only allocate the new version of the string if it actually needs to be modified, instead of eagerly build the new string to throw it away in case reduced_string its equal to raw_content (which might be the majority of the cases, since most string literals should not need rewriting)

crates/rome_js_formatter/src/utils/string_utils.rs Outdated Show resolved Hide resolved
@ematipico ematipico temporarily deployed to aws July 1, 2022 15:38 Inactive
@ematipico
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    388.9±6.38ms     6.7 MB/sec    1.01    392.5±6.19ms     6.6 MB/sec
formatter/compiler.js                    1.00    241.5±2.95ms     4.3 MB/sec    1.02    245.6±2.82ms     4.3 MB/sec
formatter/d3.min.js                      1.00    204.5±1.29ms  1312.3 KB/sec    1.02    209.6±2.46ms  1280.8 KB/sec
formatter/dojo.js                        1.00     12.8±0.01ms     5.4 MB/sec    1.02     13.1±0.03ms     5.2 MB/sec
formatter/ios.d.ts                       1.00    256.9±3.87ms     7.3 MB/sec    1.01    259.1±3.77ms     7.2 MB/sec
formatter/jquery.min.js                  1.00     48.8±0.47ms  1735.5 KB/sec    1.03     50.2±0.42ms  1685.9 KB/sec
formatter/math.js                        1.00    399.2±3.46ms  1660.9 KB/sec    1.01    405.0±3.75ms  1637.4 KB/sec
formatter/parser.ts                      1.00      8.5±0.01ms     5.7 MB/sec    1.01      8.6±0.01ms     5.6 MB/sec
formatter/pixi.min.js                    1.00    224.1±3.34ms  2005.4 KB/sec    1.01    225.4±1.50ms  1994.0 KB/sec
formatter/react-dom.production.min.js    1.00     61.5±0.59ms  1917.2 KB/sec    1.03     63.5±0.77ms  1854.9 KB/sec
formatter/react.production.min.js        1.00      3.1±0.00ms  2024.9 KB/sec    1.01      3.2±0.01ms  1999.3 KB/sec
formatter/router.ts                      1.00      6.3±0.03ms     9.4 MB/sec    1.01      6.4±0.01ms     9.3 MB/sec
formatter/tex-chtml-full.js              1.00    517.7±4.20ms  1802.5 KB/sec    1.02    525.5±4.25ms  1775.6 KB/sec
formatter/three.min.js                   1.00    258.9±2.51ms     2.3 MB/sec    1.02    264.5±3.30ms     2.2 MB/sec
formatter/typescript.js                  1.10  1612.3±29.85ms     5.9 MB/sec    1.00  1467.7±29.40ms     6.5 MB/sec
formatter/vue.global.prod.js             1.00     81.6±1.81ms  1511.8 KB/sec    1.04     84.8±2.05ms  1454.9 KB/sec

@ematipico ematipico merged commit 957bb33 into main Jul 4, 2022
@ematipico ematipico deleted the feature/remove-unnecessary-escape branch July 4, 2022 07:39
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.

Remove unnecessary escapes from string literals
2 participants