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(ast/estree): ESTree serializer use CodeBuffer #9331

Open
wants to merge 1 commit into
base: 02-24-refactor_estree_make_itoa_dependency_optional
Choose a base branch
from

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Feb 24, 2025

ESTree serializer use CodeBuffer to assemble JSON string. CodeBuffer has a more efficient push implementation than Vec<u8>.

Copy link
Contributor Author

overlookmotel commented Feb 24, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@overlookmotel overlookmotel marked this pull request as ready for review February 24, 2025 11:13
@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Feb 24, 2025
Copy link

codspeed-hq bot commented Feb 24, 2025

CodSpeed Performance Report

Merging #9331 will not alter performance

Comparing 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ (2c8281a) with main (8bde4e3)

Summary

✅ 33 untouched benchmarks

@Boshen
Copy link
Member

Boshen commented Feb 24, 2025

No difference in performance?

@overlookmotel
Copy link
Contributor Author

Only +0.5% improvement. Damn, I hoped for more.

I guess the improvement in perf of push is insignificant in context of the rest of the work the serializer does.

@Boshen
Copy link
Member

Boshen commented Feb 24, 2025

Shall we reject the PR given the added dependency? oxc_data_structures will grow in size ... hence slower compile time overall due to oxc_ast -> oxc_estree -> oxc_data_structures.

@overlookmotel overlookmotel force-pushed the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch from baa24ca to f0c4209 Compare February 24, 2025 16:52
@overlookmotel overlookmotel changed the base branch from main to graphite-base/9331 February 24, 2025 17:20
@overlookmotel overlookmotel force-pushed the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch from f0c4209 to 3b06cc3 Compare February 24, 2025 17:20
@overlookmotel overlookmotel changed the base branch from graphite-base/9331 to 02-24-refactor_estree_make_itoa_dependency_optional February 24, 2025 17:21
@overlookmotel overlookmotel force-pushed the 02-24-refactor_estree_make_itoa_dependency_optional branch from d69b85c to ddabad0 Compare February 24, 2025 17:25
@overlookmotel overlookmotel force-pushed the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch from 3b06cc3 to 0601b7d Compare February 24, 2025 17:25
@overlookmotel overlookmotel force-pushed the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch from 0601b7d to 2c8281a Compare February 24, 2025 17:40
@overlookmotel
Copy link
Contributor Author

Personally I still think it's worthwhile, for 3 reasons:

  1. CodeBuffer can be optimized further, and it'd be ideal to do that in one place, so both ESTree serialization and oxc_codegen both benefit.
  2. This PR removes near-identical code from oxc_estree (Buffer was same thing as CodeBuffer, just with less thorough doc comments!).
  3. There's a sizeable perf improvement in this PR to generation of pretty-printed JSON. We won't see that in benchmarks as they only use compact JSON serializer, but it should gain us a little bit on time to run conformance.

However, you're absolutely right about the dependency chain. I had overlooked that oxc_data_structures can be an optional dependency for oxc_estree. Have changed that now. So, unless serialize feature is enabled, it adds no dependencies to oxc_ast. (#9338 also makes itoa dependency optional - I'd also missed that before)

But I've moved this PR to the top of the stack, so that we can drop it if you feel strongly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants