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.

Sign up for GitHub

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

Jump to bottom

perf(ast/estree): ESTree serializer use CodeBuffer #9331

Merged
merged 1 commit into from
Feb 25, 2025
Merged

perf(ast/estree): ESTree serializer use CodeBuffer #9331

merged 1 commit into from
Feb 25, 2025

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Feb 24, 2025 •
edited
Loading

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

@overlookmotel Graphite App
Copy link
Contributor Author

overlookmotel commented Feb 24, 2025 •
edited
Loading

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
@overlookmotel overlookmotel mentioned this pull request Feb 24, 2025
Merged
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Feb 24, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #9331 will not alter performance

Comparing 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ (018c523) with main (fc46218)

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
This was referenced Feb 24, 2025
Closed
Merged
@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
This was referenced Feb 24, 2025
Merged
Merged
@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 2 times, most recently 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.

@overlookmotel overlookmotel mentioned this pull request Feb 24, 2025
Merged
@overlookmotel
Copy link
Contributor Author

(see also next PR #9340 which has a much bigger perf impact)

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Feb 25, 2025
@graphite-app Graphite App
Copy link

graphite-app bot commented Feb 25, 2025 •
edited
Loading

Merge activity

@overlookmotel
perf(ast/estree): ESTree serializer use CodeBuffer (#9331)
018c523 
`ESTree` serializer use `CodeBuffer` to assemble JSON string. `CodeBuffer` has a more efficient `push` implementation than `Vec<u8>`.
graphite-app bot pushed a commit that referenced this pull request Feb 25, 2025
@overlookmotel
refactor(estree): make itoa dependency optional (#9338)
2faabe1 
Make `itoa` dependency of `oxc_estree` optional. It's only required if `serialize` feature is enabled.

(as pointed out in #9331 (comment), we want to avoid `oxc_ast` crate having unnecessary dependencies)
@graphite-app graphite-app bot force-pushed the 02-24-refactor_estree_make_itoa_dependency_optional branch from 38548ac to 2faabe1 Compare February 25, 2025 04:40
@graphite-app graphite-app bot force-pushed the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch from 2c8281a to 018c523 Compare February 25, 2025 04:41
Base automatically changed from 02-24-refactor_estree_make_itoa_dependency_optional to main February 25, 2025 05:01
@graphite-app graphite-app bot merged commit 018c523 into main Feb 25, 2025
29 checks passed
@graphite-app graphite-app bot deleted the 02-23-perf_ast_estree_estree_serializer_use_codebuffer_ branch February 25, 2025 05:02
@oxc-bot oxc-bot mentioned this pull request Feb 26, 2025
Merged
Boshen added a commit that referenced this pull request Feb 26, 2025
@oxc-bot @Boshen
release(crates): v0.53.0 (#9381)
41b3d2b 
## [0.53.0] - 2025-02-26

- #9289
- 4a5a7cf napi/parser: [**BREAKING**] Remove magic string; enable utf16
span converter by default (#9291) (Boshen)

### Features

- 5c775ea ast/estree: Enable serialization without TS fields (#9285)
(overlookmotel)
- f21740e data_structures: Add `CodeBuffer::print_bytes_iter_unchecked`
method (#9337) (overlookmotel)
- e10fb97 ecmascript: Improve may_have_side_effects for `.length`
(#9366) (sapphi-red)
- 35e5ca9 ecmascript: Improve may_have_side_effects for `instanceof`
(#9365) (sapphi-red)
- 11012c6 ecmascript: Improve ValueType for coalesce operator (#9354)
(sapphi-red)
- b7998fd ecmascript: To_number for object without toString (#9353)
(sapphi-red)
- e51d563 minifier: Concatenate strings with template literals on right
side (#9356) (sapphi-red)
- 9d7db54 minifier: Concatenate strings with template literals (#9355)
(sapphi-red)
- 835ee95 wasm: Return estree with utf16 span offsets (#9376) (Boshen)

### Bug Fixes

- 6a8f53f ast/estree: Visit `JSXOpeningFragment` and
`JSXClosingFragment` (#9342) (overlookmotel)
- e303767 ast/estree: Fix ESTree AST for imports and exports (#9282)
(overlookmotel)
- 54d59f1 data_structures: Stack types correctly report allocation size
if allocation failure during grow (#9317) (overlookmotel)
- f5c8698 ecmascript: Correct may_have_side_effects for classes (#9367)
(sapphi-red)
- d3ed128 minifier: Do not remove `=== 0` if the lhs can be NaN (#9352)
(sapphi-red)

### Performance

- 82adab9 ast/estree: Speed up building UTF8-UTF16 translation table
with SIMD (#9359) (overlookmotel)
- 61939ca ast/estree: Faster UTF-8 to UTF-16 span conversion (#9349)
(overlookmotel)
- 1bfc459 ast/estree: Pre-allocate `CodeBuffer` for JSON output (#9340)
(overlookmotel)
- 018c523 ast/estree: `ESTree` serializer use `CodeBuffer` (#9331)
(overlookmotel)
- 35ee399 codegen: Use `iter::repeat_n` in `CodeBuffer` (#9325)
(overlookmotel)

### Documentation

- 8bd3e39 data_structures: Uppercase SAFETY comments (#9330)
(overlookmotel)

### Refactor

- d94fc15 allocator: Reduce scope of `unsafe` blocks (#9319)
(overlookmotel)
- 7427900 ast: Re-order `ExportDefaultDeclaration` fields (#9348)
(overlookmotel)
- b09249c ast/estree: Rename serializers and serialization methods
(#9284) (overlookmotel)
- 55ed1df ast/estree: Shorten `ESTree` impls for enums (#9275)
(overlookmotel)
- 9d98444 codegen, data_structures: Move `CodeBuffer` into
`oxc_data_structures` crate (#9326) (overlookmotel)
- 6a4e892 data_structures: Add debug assertion to
`CodeBuffer::peek_nth_char_back` and improve safety docs (#9336)
(overlookmotel)
- fc46218 data_structures: `CodeBuffer::print_str` use
`Vec::extend_from_slice` (#9332) (overlookmotel)
- 690bae5 data_structures: Stack types const assert `T` is not zero-size
type (#9318) (overlookmotel)
- 10ba2ea data_structures: Reduce scope of `unsafe` blocks (#9316)
(overlookmotel)
- beb8382 data_structures: `CodeBuffer::print_bytes_unchecked` take a
byte slice (#9327) (overlookmotel)
- faf966f ecmascript: Don't check side effects in constant_evaluation
(#9122) (sapphi-red)
- 2faabe1 estree: Make `itoa` dependency optional (#9338)
(overlookmotel)
- 4e9e8cf lexer: Reduce scope of `unsafe` blocks (#9320) (overlookmotel)
- c31b53f mangler: Reduce scope of `unsafe` blocks (#9321)
(overlookmotel)
- f10a6da mangler: Move base54 into seperate mod (#9278) (Cameron)
- 12e89e0 syntax: Reduce scope of `unsafe` blocks (#9322)
(overlookmotel)
- f39be5f traverse: Reduce scope of `unsafe` blocks (#9323)
(overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
0-merge Merge with Graphite Merge Queue C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@overlookmotel @Boshen