Uses blink-alloc instead of bumpalo #106
Conversation
|
The test failed with with Which is fine I think, we can mark it as 24. |
|
Here are the benchmarks from the CI jobs, they aren't posted by bot due to permission issues. Parser Benchmark Results - macos-latestParser Benchmark Results - windows-latestParser Benchmark Results - ubuntu-latest |
| // one just allocated from a Bump. | ||
| unsafe { ptr::read(self.0 as *mut T) } | ||
| } | ||
| } |
There was a problem hiding this comment.
I kind of liked the unbox API, its from jsparagus
There was a problem hiding this comment.
Box::into_inner is from std.
And Box should not have methods with self to not cause name resolution conflict with pointee.
crates/oxc_cli/src/lib.rs
Outdated
| @@ -126,7 +126,7 @@ impl Cli { | |||
| let parser_source_text = source_text.clone(); | |||
| let ret = Parser::new(&allocator, &parser_source_text, source_type).parse(); | |||
| let result = if ret.errors.is_empty() { | |||
| let program = allocator.alloc(ret.program); | |||
| let program = allocator.emplace_no_drop().value(ret.program); | |||
There was a problem hiding this comment.
These emplace-* APIs are pretty new to me.
There was a problem hiding this comment.
Blink allows dropping values placed into the allocated memory.
emplace_* configure how values are placed.
|
Wait ... it seems to be slower from the benchmarks, I didn't fully wake up yet and saw it the other way around. |
crates/oxc_allocator/src/lib.rs
Outdated
| } | ||
|
|
||
| // SAFETY: Make Bump Sync and Send, it's our responsibility to never | ||
| // simultaneously mutate across threads. | ||
| unsafe impl Send for Allocator {} | ||
| unsafe impl Sync for Allocator {} | ||
|
|
||
| pub type Box<'a, T> = allocator_api2::boxed::Box<T, &'a BlinkAlloc>; | ||
| pub type Vec<'a, T> = allocator_api2::vec::Vec<T, &'a BlinkAlloc>; |
There was a problem hiding this comment.
I think this is the main piece of the puzzle I was missing.
You can annotate the lifetime regardless of allocator_api2::vec::Vec/Box having a signature "set up for it".
Makes total sense after seeing it, but this wasn't a possibility I was aware of.
|
Will need to profile if we want to understand why its slower. The internals complexity is beyond understanding with a code skim, one thing that stood out to me was was that the pub fn into_inner(boxed: Self) -> T {
let ptr = boxed.0;
let unboxed = unsafe { ptr.as_ptr().read() };
unsafe { boxed.1.deallocate(ptr.cast(), Layout::new::<T>()) };
unboxed
}Deallocate is trying to reclaim the memory if the deallocated object is the last one. |
|
I was thinking of removing deallocations or making them only for large enough blocks |
|
@zakarumych Are the benchmarks wrong or is there something fishy going on? |
|
Turned out that duplicates of |
Is there anything I can help out with? |
|
@zakarumych Friendly ping :-) May I ask you what the current status is? |
|
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. |
The usage is roughly the same.
One caveat applies:
String is not generalized over allocator in std, so
allocator-api2lacks one too.There seems to be room for perf improvement.
On my machine this change gives 2.5 - 3% speed increase