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(semantic): use faster indextree building #122

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

YoniFeng
Copy link
Contributor

@YoniFeng YoniFeng commented Mar 4, 2023

Related to issue #115, but not necessarily the final change.

Local profiling shows improvement as expected (see comment in the issue), which isn't large/significant.
I opened an indextree PR, will need to wait for feedback.

@Boshen
Copy link
Member

Boshen commented Mar 5, 2023

I'll setup the CI benchmarks today. I also invited you to the repo so github can post benchmark results.

@Boshen Boshen marked this pull request as draft March 5, 2023 02:58
@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 5, 2023

Running the linter on typescript.js (which has 3 lint errors and runs <1s):
main
image
You can see the top hit's callstack originates in create_ast_node() -> append()

pr
image

Notice I couldn't figure out how to group based on "indextree"/call stack.
If I manually sum everything >1ms originating in append vs append_new_node(the pr version), the reduction is from 26ms to 13ms.

A few notes:

  1. This improvement is only noticeable with no/few errors. When there are many errors, miette used to print the diagnostics dominates time/CPU usage by an order of magnitude, making the linting work itself negligible.
    (The screenshot profiles were run linting typescript.js. It's rather large, but has a lot of comments (which aren't added to the AST, right?) and only 3 lint errors. When I run for babylon.max.js (which has hundreds of errors), miette dominates all time significantly.)
  2. These are one-off profiles. A proper benchmark will give an indication of actual bottom-line gain.

@Boshen
Copy link
Member

Boshen commented Mar 5, 2023

The CI actions has proper benchmarks now https://github.com/Boshen/oxc/actions/runs/4336428902

The numbers are really significant!

image

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 5, 2023

Ha I just noticed right before you posted.
I rebased on main to make sure the parser isn't actually regressed.

Cargo.toml Outdated Show resolved Hide resolved
@Boshen
Copy link
Member

Boshen commented Mar 5, 2023

I rebased on main to make sure the parser isn't actually regressed.

~2% change is just noise.


Shall we wait for a week for indextree or do we start looking for alternatives?

@Boshen
Copy link
Member

Boshen commented Mar 5, 2023

I can use your clone for the time being, since indextree is rarely updated.

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 5, 2023

Let's wait.
I believe indextree's maintainer will work tomorrow and respond.

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 9, 2023

Squashed, ready for merge

saschagrunert/indextree#92 was merged

@Boshen
Copy link
Member

Boshen commented Mar 9, 2023

https://github.com/Boshen/oxc/blob/bc2c17547291b1127bd4ec1feb2c6c8f4ebc027e/crates/oxc_semantic/src/scope/builder.rs#L39-L40

The scope tree is also an indextree, let's update it as well and see the overall benchmark result.

@Boshen Boshen marked this pull request as ready for review March 9, 2023 08:27
@Boshen Boshen changed the title (perf) use faster indextree building perf(semantic): use faster indextree building Mar 9, 2023
@Boshen
Copy link
Member

Boshen commented Mar 9, 2023

Parser Benchmark Results - ubuntu-latest

semantic/babylon.max.js    1.73    244.0±6.85ms    42.3 MB/sec    1.00    141.0±7.08ms    73.2 MB/sec
semantic/d3.js             1.97     19.8±0.38ms    27.6 MB/sec    1.00     10.1±0.46ms    54.2 MB/sec
semantic/lodash.js         2.25      5.7±0.13ms    89.9 MB/sec    1.00      2.5±0.24ms   202.3 MB/sec
semantic/pdf.js            2.17     14.9±0.67ms    27.0 MB/sec    1.00      6.8±0.20ms    58.7 MB/sec
semantic/typescript.js     1.64    160.5±7.80ms    59.9 MB/sec    1.00     97.6±6.74ms    98.5 MB/sec

@Boshen Boshen merged commit 8f26b99 into oxc-project:main Mar 9, 2023
@Boshen
Copy link
Member

Boshen commented Mar 9, 2023

Thank you so much for working on this @YoniFeng I'll praise you whenever I can.

@Boshen Boshen mentioned this pull request Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants