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,ast): add BoxWithDrop<BigInt> #2201

Closed
wants to merge 1 commit into from

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Jan 29, 2024

relates #1803

I wanna make things work first, get miri to pass, and then work on better workarounds. cc @overlookmotel

Miri run: https://github.com/oxc-project/oxc/actions/runs/7693186484

@github-actions github-actions bot added A-parser Area - Parser A-ast Area - AST A-codegen Area - Code Generation A-prettier Area - Prettier labels Jan 29, 2024
@Boshen Boshen marked this pull request as ready for review January 29, 2024 08:32
@Boshen Boshen marked this pull request as draft January 29, 2024 08:32
@Boshen
Copy link
Member Author

Boshen commented Jan 29, 2024

Ohhh... this won't work, the drop calls need to be chained all the way to the top :-) I can't just stick a BoxWithDrop in the middle of no where and expect it's drop to be called.

Copy link

codspeed-hq bot commented Jan 29, 2024

CodSpeed Performance Report

Merging #2201 will not alter performance

⚠️ No base runs were found

Falling back to comparing bigint-box-with-drop (fb3d877) with main (56adfb1)

Summary

✅ 17 untouched benchmarks

@Boshen Boshen closed this Jan 29, 2024
@Boshen Boshen deleted the bigint-box-with-drop branch January 29, 2024 13:35
@overlookmotel
Copy link
Contributor

The best solution I think is to prevent BigInt from making allocations in the first place. Unfortunately that'd require reimplementing BigInt (or at least the parts of its API which OXC uses).

To do a swifter fix in the meantime, the only way I can see to fix the memory leak without making the entire AST drop is:

  1. Store pointers to all BigInts in a vec in Allocator.
  2. Add impl Drop to Allocator which converts those pointers back to BigInts and drops them.

That would solve the leak, though I'm not sure it'd be sound - the pointers would need to be *mut, so it might well be violating Rust's aliasing rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-codegen Area - Code Generation A-parser Area - Parser A-prettier Area - Prettier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants