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

refactor(rome_js_formatter): avoid snapshot when formatting skipped token trivia #2683

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jun 8, 2022

Summary

This removes the need to take a snapshot before formatting leading trivia by collecting the comments into a Vec. The performance characteristics of the new implementation should be similar to the old because it removes the need to allocate a temporary VecBuffer to be able to reverse the written comments in the end.

I find this solution slightly easier to reason about and results in slightly less code:

Before:

  • 3.5% 10.9% 422.4KiB rome_js_formatter
  • 0.1% 0.4% 9.7KiB rome_js_formatter <rome_js_formatter::builders::FormatLeadingTrivia as rome_formatter::Format<rome_js_formatter::context::JsFormatContext>>::fmt

After:

  • 3.3% 10.6% 407.1KiB rome_js_formatter
  • FormatLeadingTrivia no longer shows up.

It also gives us the option to use a SmallVec to store comments if profiling shows that this is beneficial.

Test Plan

cargo test

@MichaReiser MichaReiser force-pushed the refactor/skipped-trivia-formatting branch from 691dea5 to a83af32 Compare June 8, 2022 09:52
@MichaReiser MichaReiser temporarily deployed to aws June 8, 2022 09:52 Inactive
@MichaReiser MichaReiser force-pushed the refactor/skipped-trivia-formatting branch from a83af32 to 02eb10b Compare June 8, 2022 09:54
@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 8, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 648cdb9
Status: ✅  Deploy successful!
Preview URL: https://7a66e057.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser temporarily deployed to aws June 8, 2022 09:55 Inactive
@github-actions
Copy link

github-actions bot commented Jun 8, 2022

@github-actions
Copy link

github-actions bot commented Jun 8, 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 350 350 0
Failed 5595 5595 0
Panics 1 1 0
Coverage 5.89% 5.89% 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 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

@MichaReiser MichaReiser force-pushed the refactor/skipped-trivia-formatting branch from 02eb10b to 9245d79 Compare June 8, 2022 10:09
@MichaReiser MichaReiser temporarily deployed to aws June 8, 2022 10:09 Inactive
@ematipico ematipico changed the title refactor(formatter): Avoid snapshot when formatting skipped token trivia refactor(rom_formatter): avoid snapshot when formatting skipped token trivia Jun 8, 2022
@ematipico ematipico changed the title refactor(rom_formatter): avoid snapshot when formatting skipped token trivia refactor(rome_js_formatter): avoid snapshot when formatting skipped token trivia Jun 8, 2022
@MichaReiser
Copy link
Contributor Author

!bench_formatter

@MichaReiser MichaReiser force-pushed the refactor/skipped-trivia-formatting branch from 9245d79 to 648cdb9 Compare June 8, 2022 11:53
@MichaReiser MichaReiser temporarily deployed to aws June 8, 2022 11:53 Inactive
@github-actions
Copy link

github-actions bot commented Jun 8, 2022

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00   361.4±14.45ms     7.2 MB/sec    1.02   368.9±19.69ms     7.0 MB/sec
formatter/compiler.js                    1.00   211.4±11.69ms     5.0 MB/sec    1.02    215.4±7.66ms     4.9 MB/sec
formatter/d3.min.js                      1.00    157.7±8.01ms  1701.5 KB/sec    1.09    171.3±7.76ms  1567.2 KB/sec
formatter/dojo.js                        1.00     11.1±0.60ms     6.2 MB/sec    1.00     11.2±0.40ms     6.2 MB/sec
formatter/ios.d.ts                       1.08    258.2±9.15ms     7.2 MB/sec    1.00   238.2±11.19ms     7.8 MB/sec
formatter/jquery.min.js                  1.00     48.9±2.95ms  1730.6 KB/sec    1.05     51.4±3.35ms  1647.2 KB/sec
formatter/math.js                        1.01   372.4±13.95ms  1780.8 KB/sec    1.00   367.7±16.99ms  1803.1 KB/sec
formatter/parser.ts                      1.06      7.7±0.27ms     6.3 MB/sec    1.00      7.2±0.26ms     6.7 MB/sec
formatter/pixi.min.js                    1.00    181.3±9.61ms     2.4 MB/sec    1.05    190.8±6.34ms     2.3 MB/sec
formatter/react-dom.production.min.js    1.00     50.9±2.42ms     2.3 MB/sec    1.04     53.1±2.32ms     2.2 MB/sec
formatter/react.production.min.js        1.14      3.0±0.28ms     2.0 MB/sec    1.00      2.7±0.12ms     2.3 MB/sec
formatter/router.ts                      1.09      6.3±0.29ms     9.7 MB/sec    1.00      5.8±0.29ms    10.6 MB/sec
formatter/tex-chtml-full.js              1.04   494.5±22.05ms  1887.1 KB/sec    1.00   475.2±12.96ms  1963.9 KB/sec
formatter/three.min.js                   1.06    228.4±9.71ms     2.6 MB/sec    1.00    214.9±7.21ms     2.7 MB/sec
formatter/typescript.js                  1.03  1373.3±63.45ms     6.9 MB/sec    1.00  1339.1±48.85ms     7.1 MB/sec
formatter/vue.global.prod.js             1.07     77.5±2.21ms  1592.0 KB/sec    1.00     72.1±3.44ms  1710.7 KB/sec

@MichaReiser MichaReiser merged commit ead9498 into main Jun 9, 2022
@MichaReiser MichaReiser deleted the refactor/skipped-trivia-formatting branch June 9, 2022 11:02
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.

None yet

3 participants