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

Improved performance with large HTML files #11696

Merged
merged 22 commits into from Jan 11, 2022

Conversation

sapphi-red
Copy link
Sponsor Contributor

@sapphi-red sapphi-red commented Oct 20, 2021

Description

I reduced copying AST like #11108.

On this case, the result of yarn run perf:benchmark became about 8.6x faster.

before

[debug] resolve config from 'test.html'
[debug] loaded options `{"useTabs":false,"tabWidth":2,"endOfLine":"lf"}`
[debug] applied config-precedence (cli-override): {"endOfLine":"lf","tabWidth":2,"useTabs":false}
[debug] '--debug-benchmark' option found, measuring formatWithCursor with 'benchmark' module.
[debug] '--debug-benchmark' measurements for formatWithCursor: {
[debug]   "benchmark": "format x 0.80 ops/sec ±3.60% (7 runs sampled)",
[debug]   "hz": 0.8011225145560241,
[debug]   "ms": 1248.2485285714286
[debug] }

after

[debug] resolve config from 'test.html'
[debug] loaded options `{"useTabs":false,"tabWidth":2,"endOfLine":"lf"}`
[debug] applied config-precedence (cli-override): {"endOfLine":"lf","tabWidth":2,"useTabs":false}
[debug] '--debug-benchmark' option found, measuring formatWithCursor with 'benchmark' module.
[debug] '--debug-benchmark' measurements for formatWithCursor: {
[debug]   "benchmark": "format x 6.86 ops/sec ±11.89% (23 runs sampled)",
[debug]   "hz": 6.86268975187975,
[debug]   "ms": 145.71546086956522
[debug] }

refs #4776

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker
Copy link
Sponsor Member

fisker commented Nov 5, 2021

Thanks for working on this.

the result of yarn run perf:benchmark became about 8.6x faster.

👍

@sapphi-red
Copy link
Sponsor Contributor Author

@fisker Is there anything I should do for the next step?

src/language-html/ast.js Outdated Show resolved Hide resolved
@sapphi-red
Copy link
Sponsor Contributor Author

I have changed the code to use for loop to recalculate index/next/prev as you suggested, which would be much more readable!
The benchmark result for this code is below.

[debug] resolve config from 'test.html'
[debug] loaded options `{"useTabs":false,"tabWidth":2,"endOfLine":"lf"}`
[debug] applied config-precedence (cli-override): {"endOfLine":"lf","tabWidth":2,"useTabs":false}
[debug] '--debug-benchmark' option found, measuring formatWithCursor with 'benchmark' module.
[debug] '--debug-benchmark' measurements for formatWithCursor: {
[debug]   "benchmark": "format x 5.21 ops/sec ±10.93% (19 runs sampled)",
[debug]   "hz": 5.207000038723637,
[debug]   "ms": 192.04916315789475
[debug] }

src/language-html/ast.js Outdated Show resolved Hide resolved
src/language-html/ast.js Outdated Show resolved Hide resolved
Co-authored-by: fisker Cheung <lionkay@gmail.com>
@fisker
Copy link
Sponsor Member

fisker commented Jan 7, 2022

I'm not sure about the performance impact, it seems we can use NODES_KEYS as a Set instead of an object. There are lots of for..in loop.

@fisker
Copy link
Sponsor Member

fisker commented Jan 7, 2022

I'm not familiar with this part of code. Why are we using descriptors instead of class getters?

@sapphi-red
Copy link
Sponsor Contributor Author

I'm not sure about the performance impact, it seems we can use NODES_KEYS as a Set instead of an object. There are lots of for..in loop.

I tried this now but there was no significant difference (using Set was only about 1ms slower).

@sapphi-red
Copy link
Sponsor Contributor Author

I'm not familiar with this part of code. Why are we using descriptors instead of class getters?

I'm not sure.
I thought there is no big reason so I rewrote it to class getters and properties.

commit
008975f 136.69717916666664ms x 7.32 ops/sec ±13.22%
00099ca 135.36796250000003ms x 7.39 ops/sec ±11.86%
f6096d2 142.9828695652174ms x 6.99 ops/sec ±11.72%
eb0b59f 141.92638695652172ms x 7.05 ops/sec ±11.45%
e9adae2 160.52020476190478ms x 6.23 ops/sec ±11.29%
381a73c 175.88805ms x 5.69 ops/sec ±10.47%

src/language-html/ast.js Outdated Show resolved Hide resolved
src/language-html/ast.js Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@duailibe duailibe merged commit dd3e1a4 into prettier:main Jan 11, 2022
@sapphi-red sapphi-red deleted the html-reduce-ast-copy branch January 14, 2022 10:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2023
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