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(parser): try Blink allocator for the AST #88

Closed
Boshen opened this issue Mar 1, 2023 · 9 comments
Closed

perf(parser): try Blink allocator for the AST #88

Boshen opened this issue Mar 1, 2023 · 9 comments
Labels
C-enhancement Category - New feature or request

Comments

@Boshen
Copy link
Member

Boshen commented Mar 1, 2023

https://twitter.com/zakarum4/status/1630669810110545921

https://github.com/zakarumych/blink-alloc

cc @zakarumych

@Boshen Boshen added the C-enhancement Category - New feature or request label Mar 1, 2023
@Boshen Boshen added this to the AST / Lexer / Parser milestone Mar 1, 2023
@Boshen
Copy link
Member Author

Boshen commented Mar 1, 2023

@zakarumych I've gone through the docs (blink_alloc and allocator-api2) and I'm confused, it's probably because I'm unfamiliar with the allocator api.

So given I have nightly on, how do I go about replacing bumpalo?

And will this remove all the lifetimes from the AST (which would be awesome)?

@zakarumych
Copy link

allocator-api2 mimics traits and types from core and alloc to enable usage on stable.
With "nightly" feature enabled allocator-api2 reexports instead of defining duplicates, so core and alloc traits and types may be used directly if you target nightly only.
If you target both nightly and stable - use allocator-api2.

No. You won't get rid of lifetimes easily since BlinkAlloc should shared between containers rather than cloned.
There is a way to get rid of lifetimes if you really need this.

Implement unsafe reset_unchecked method that takes &self. Place BlinkAlloc into static and voila, &'static BlinkAlloc gives 'static allocations.
Reset would be unsafe since it would require caller that all allocations were dropped.

@Boshen
Copy link
Member Author

Boshen commented Mar 1, 2023

@zakarumych Would it be too much of an ask for you to add some building blocks for me? I'm super confused :-/

@zakarumych
Copy link

What seems to be missing?

@YoniFeng
Copy link
Contributor

YoniFeng commented Mar 3, 2023

Prefacing the below with: I'm new to Rust, and I'm also not deeply familiar with this project's usage of arena allocations.

(@Boshen please correct any mistakes)
For suitable use cases, using an arena allocator can yield better performance. (thanks lower (de)allocation overhead, spatial locality).
In the case of this oxc project, we know the lexer and the AST builder are going to use memory which is "temporary" in the sense that once used (output JS or lint) - will be thrown away entirely.

To get this to work within Rust's lifetime constraints, the allocator(bumpalo) is created and passed to the parser's constructor.
This enables storing references to the allocator-allocated objects (since the allocator, which owns the large continuous memory, is shown to outlive its use).

It seems there's a general incompatibility between the utilities and function signatures bumpalo provides vs blink.
bumpalo's alloc() is

pub fn alloc<T>(&self, val: T) -> &mut T

blink's put() is

 pub fn put<T: 'static>(&self, value: T) -> &mut T

(I'm entirely confused by the static lifetime)

bumpalo's Vec is

pub struct Vec<'bump, T: 'bump>

blink's is:

pub struct Vec<T, A: Allocator = Global>

Trying to replace bumpalo usage with blink comes up at a dead-end, where it seems to me the API's exposed by blink aren't sufficient (and it's as if I should go implement it - except I have no idea if/how it fits in with Blink, from a "provider"'s perspective).
Since you've put in great care and effort, I assume I'm misunderstanding the intended usage and why/how it's different than bumpalo's.

@Boshen
Copy link
Member Author

Boshen commented Mar 4, 2023

@YoniFeng Your understanding of how bumaplo is being used in this project is correct.

@zakarumych Can you help us a little more on how to make Box and Vec work?

I presume we start replacing this with Blink:

https://github.com/Boshen/oxc/blob/47c1e7ef5124b81d6df0cd8182b56a94e297a04b/crates/oxc_allocator/src/lib.rs#L8-L11

And we want to end up with the replacement of these APIs with the Blink's equivalent:

https://github.com/Boshen/oxc/blob/47c1e7ef5124b81d6df0cd8182b56a94e297a04b/crates/oxc_ast/src/ast_builder.rs#L19-L46


Please excuse us for our unfamiliarity around this topic.

@Boshen Boshen changed the title perf(parser): try another allocator for the AST perf(parser): try Blink allocator for the AST Mar 4, 2023
@Boshen
Copy link
Member Author

Boshen commented Mar 4, 2023

Oops, I didn't see the PR #106 first, I'm going to look at it now.

@zakarumych
Copy link

Blink::put requires 'static values due to placing it into drop-list.
Without 'static it would be possible to observe destroyed objects in value's Drop.
There are 3 ways to relax 'static requirement.

Blink::emplace_no_drop() works as Bump::alloc. i.e. it doesn't drop the value on reset, removing possibility to witness dropped values in Drop.

Blink::emplace_shared() gives only shared reference back and removes the problem as well.
Blink::emplace_unchecked() is unsafe and asks caller to not cause UB :)

As you've seen in a PR a type-alias helps to match bumpalo's Box and Vec types.

@Boshen
Copy link
Member Author

Boshen commented Mar 29, 2023

Closing due to inactivity. Please reach out if you want to work on this again. And thank you for working on this, I learned a bunch from your work.

@Boshen Boshen closed this as not planned Won't fix, can't repro, duplicate, stale Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants