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

feat(allocator)!: replace bumpalo with bump-scope #6668

Closed

Conversation

bluurryy
Copy link

@bluurryy bluurryy commented Oct 18, 2024

This is just a test to get a benchmark result.

Copy link

graphite-app bot commented Oct 18, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

@github-actions github-actions bot added A-parser Area - Parser A-ast Area - AST A-transformer Area - Transformer / Transpiler A-prettier Area - Prettier labels Oct 18, 2024
@Boshen Boshen changed the title Replaced dependency bumpalo with bump-scope feat(allocator)!: replace bumpalo with bump-scope Oct 18, 2024
@github-actions github-actions bot added the C-enhancement Category - New feature or request label Oct 18, 2024
Copy link

codspeed-hq bot commented Oct 18, 2024

CodSpeed Performance Report

Merging #6668 will not alter performance

Comparing bluurryy:replace-bumpalo-with-bump-scope (3f81ffb) with main (4cf0085)

Summary

✅ 30 untouched benchmarks

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-cli Area - CLI A-minifier Area - Minifier A-codegen Area - Code Generation A-cfg Area - Control Flow Graph A-isolated-declarations Isolated Declarations A-ast-tools Area - AST tools A-editor Area - Editor and Language Server labels Oct 19, 2024
@Boshen Boshen marked this pull request as ready for review October 26, 2024 03:54
@overlookmotel
Copy link
Contributor

overlookmotel commented Oct 26, 2024

It's interesting that we're seeing improvement on some benchmarks (transformer, minifier, codegen), and regression on others (parser). That split roughly corresponds to crates that mostly read from arena (transformer etc) and crates that mostly write (parser).

A few thoughts:

Bench allocator separately

My suspicion (which could be wrong) is that the perf improvement is coming from Vec being more optimized, and the regression is coming from the allocator itself.

Would it be possible to reduce the scope of this PR to just replacing the allocator itself, keeping our existing Box, Vec and String as is? That'd allow us to see the effect of replacing the allocator in isolation.

My understanding is that bump_scope::Bump implements allocator_api2::alloc::Allocator, so our existing Vec etc should be compatible with it? Obviously I'm not saying to throw away the work you've done on Vec etc, but just to put it aside for another PR.

Judging by the more efficient allocation implementation you claim for bump-scope vs bumpalo, replacing the allocator alone should still be a perf boost. If it isn't, something is wrong.

Bump downwards

I suggest switching to bumping downwards. That should be faster (as I know you're aware) and also follows bumpalo more closely. I'm guessing bumping upwards is what's causing the perf regressions in this PR.

Minimum alignment

Let's leave MINIMUM_ALIGNMENT = 1 for now. Likely increasing it to size_of::<usize>() will give a perf boost as almost all our AST types are either aligned on 8 or have size which a multiple of 8 (on 64-bit platforms) . The only exceptions I'm aware of are BooleanLiteral (size 12, alignment 4) and strings (alignment 1).

We generally don't allocate many strings, as most strings are stored just as references to slices of the source text.

We might in future want to store strings in a separate arena with MINIMUM_ALIGNMENT = 1. That arena would probably bump upwards to allow building strings directly in arena without memory copying when the string grows. Or, rather then 2 arenas, we could adapt allocator to fill chunks from both ends (oxc-project/backlog#37), with strings filling from the start (upwards), and everything else filling from the end (downwards).

But, again, any such experiments would be better left to separate PRs.

Vec

Please note that we intend to replace Vec with a hand-written version anyway. We can:

  1. Use u32s for len and capacity, to reduce size of Vec from 32 to 24 bytes (test: reduce size of vec #6488 showed that's a significant perf boost).
  2. Remove all code related to Drop. We only store non-drop types in arena, so this code is pure overhead. In some cases, I don't think compiler can optimize it out either e.g. IntoIter could be much smaller if you don't have to worry about drop.
  3. Further reduce size of Vec to 16 by encoding the pointer to Bump in the other fields (Reduce size of Vec backlog#18 (comment)).

…`, implement `String` as a wrapper around `Vec`, bump downwards
@bluurryy
Copy link
Author

I reverted Vec to the way it was, beign implemented on top of allocator_api2::vec::Vec. The diff to main should now be minimal and the benchmark should only show the allocator difference.

I did need to add a String implementation here, because we were using String from bumpalo, which won't work anymore (allocator_api2 has no String; probably because std has no allocator parameter for String).

The bump allocator will now bump downwards.

@Boshen Boshen marked this pull request as draft November 21, 2024 08:08
@Boshen
Copy link
Member

Boshen commented Nov 24, 2024

Thank you for working on this. Close as stale, but feel free to come back and make improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-ast-tools Area - AST tools A-cfg Area - Control Flow Graph A-cli Area - CLI A-codegen Area - Code Generation A-editor Area - Editor and Language Server A-isolated-declarations Isolated Declarations A-linter Area - Linter A-minifier Area - Minifier A-parser Area - Parser A-prettier Area - Prettier A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants