-
-
Notifications
You must be signed in to change notification settings - Fork 495
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
Memory leak in arena with Atom
s
#1803
Comments
For similar reasons, Any other structures which store data on the heap will also leak, though I couldn't see any other cases at first glance. |
Thank you for the in-depth research. For strings, my previous attempt was #451, I think we can look into this problem space a bit more to find a good solution. For big ints, the usages are rare - perhaps we could box it into the arena? @overlookmotel Would you like to explore and find a way to mitigate these memory leak problems with us? Thank you in advance 🙇 |
Let's start with option 3 (replace compact string). I still want to try the hack from https://github.com/rust-lang/rust/blob/f9097f87c9c094f80826fb60a1a624b5f9f1ed82/compiler/rustc_span/src/symbol.rs#L1999-L2007. |
This is going to be fun bug ... Maybe there is a usage problem in our lexer code? |
First go at a new A few questions: MutablilityAre there plans/potential needs in future for What I've done so far assumes not, and therefore can reduce size of Use references?Should all strings be added to the arena? Or if there's an existing reference to the string's content whose lifetime is long enough, can Advantages of storing references:
Advantage of putting everything in the arena is that strings will always be close in arena to the AST nodes which use them, so better cache locality. Then again, strings also bloat the arena, so possibly result in more swapping in and out of the cache while traversing an AST. My current WIP is based on storing references. Support for 32-bit systemsHow important is support for 32-bit systems? What I've done so far produces a maximum cap on length of any individual string of 512 MiB on 32-bit machines (no practical limit for 64-bit). Personally, I imagine 512 MiB is big enough for almost all use cases. And does anyone actually use 32-bit machines any more? |
On your other points: I'm not sure I understand the point of the hack you mentioned in this context. As far as I can see, the reason that code is tricking the compiler that strings have a It needs to do that because it's de-duplicating strings. But I thought you'd found that approach was slower due to the locks and hash table lookups, and discounted it? Or am I missing the point? I figured out why string content appears 3 times in the arena. It's because bumpalo has the unfortunate property that it grows downward. i.e. in each chunk the first allocation is at end of the chunk, the next allocation before it, and so on. So when I imagine this same effect also occurs when building any other My first thought is this could be improved on by using a separate "scratch space" allocation which can grow forwards. |
No, I haven't encountered a usecase where it needs to be mutable.
I wanted to do this for a long time. But handling out If we can hack it and unsafe the
Your reasoning is solid around 512 MiB 👍
It's basically this comment:
removing the lifetime will make all downstream code easier to write, since people don't need to carry around the Although we can force an allocation for external use ... so everything is still safe Rust.
TIL. We can create a separate discussion around this topic. Could be fun to optimize it, or perhaps wasting a lot of time 😅 |
Thanks for coming back.
Hmm. Well yes we could hack it, but then wouldn't it be unsound? You could drop the I'm also a little confused as to why Also, If the problem is that the source code is stored separately from the arena, so it produces a 2nd lifetime to deal with, how about let src = "x = 123".to_string();
let alloc = Allocator::from_src(&src);
// `src` must live as long as `alloc` I think it'd be a shame to leave this potential performance gain unrealised, as it could be significant for any code containing long strings (which would include a lot of UI code). Out of interest, what downstream tools are you talking about? Other OXC crates, or external consumers?
Solving this problem in |
How about this - we can expose different APIs for different usages, i.e.
Out of interest, what downstream tools are you talking about? Other OXC crates, or external consumers? All crates other than the AST crate are considered external crates:
I love the overall direction this issue is going! @overlookmotel do you wanna find a way to hack some of the code so we can get a PR going, and observe performance changes? Both performance and memory consumption should be improved in theory. As stated in the policy https://oxc-project.github.io/docs/contribute/rules.html#development-policy
;-) |
I think I've provided enough context for you to make good judgements, you've gained my trust. Let's break things and have fun! |
Thanks. Or maybe I've just worn you down with my questions! But yes, I think I have a fairly clear picture now. Just one clarification, and 1 last question:
To clarify, you mean that And the last question: Am I right in thinking that OXC places a limit on size of a source file to max That'd free 4 bytes in |
Yes, I think we should allow the user (other crates) to pick for their use case so everybody can make their own tradeoffs, as well as stay in safe Rust. |
Yes! oxc/crates/oxc_span/src/span.rs Line 14 in 1957770
|
OK great. Thanks for the answers. I have a WIP that ticks most of the boxes, but now that holiday is coming to an end I'll have much less time to work on it. Please excuse me if I don't come back with a PR for a while. |
Take your time, I'm on holiday as well 😁 |
Sorry, one more question: How much should we care about 32-bit systems? Are they a primary target for OXC? Reason I ask is implementation would be simpler if could make On 64-bit systems it'll be 16 bytes regardless. |
Not that much. Not a primary target. |
Thanks. I'll get on with the implementation. Will come back if more questions, but hopefully I have all the info I need now. |
Why do we need If downstream tool needs a lifetimeless string, they should be able to use |
Yes, I was thinking along similar lines last night.
But I can still see a few advantages to an
When printing an AST back to Javascript, when the printer holds an This would be the case for most identifiers, as typically they'll be under 16 bytes.
During e.g. scope analysis, I'd imagine there's a lot of comparing one identifier's name to another's. When those identifiers are 16 chars or less, that should be faster if they're stored inline because: a. The data is right there inline in the
A custom (1) and (2) are the main considerations. Basically, it's a trade-off between speed of creating strings and of reading them. Maybe I'm most of the way there with the inline Does that sound like a reasonable plan? |
Yes yes! I totally forgot that all we want is 😅 enum Atom {
Inline(..),
ArenaAllocated(..)
} |
Thanks! |
👍 I just ran into this with parsing a trivial script containing only |
@aapoalas Do you mind sharing your miri setup? I'd like to add a CI for it. My previous attempt wasn't robust enough https://github.com/oxc-project/oxc/blob/main/.github/workflows/miri.yml |
Unfortunately I only just ran the rustup script to add nightly toolchain, and then ran |
Weird, I tried If I run the whole test suite it throws something from crossbeam
https://github.com/oxc-project/oxc/actions/runs/7528975555/job/20492273972 |
Is it possible that the tests don't create any AST which contains either If the tests don't do either of these things, they won't trigger the memory leaks. I imagine running the parser on the files used for the conformance tests would allow miri to find the leaks. |
Thank you both. Replicated https://github.com/oxc-project/oxc/actions/runs/7530649712/job/20497529584
|
Found some arcane code from ratel, where we can use for our atom: |
Here's where I'm up to with It's heavily based on But thanks for link to Ratel's implementation - I'll see if anything we can use from it which is specific to strings in JS. Working on One question for now: How relaxed are you about breaking changes? I mean: Is it better to work out API design thorougly before implementing, so the breaking changes happens all in one go? Or is it OK to merge an initial implementation (including breaking changes) and then modify API again later (more breaking changes)? |
Also, a naming thing: Should we change the name from The name |
Can do since we are not 1.0. But there are many projects that dependents on oxc now, so we'll need to introduce some type alias and the |
Agree that a deprecation period is nice. Our JS engine uses Atom frankly a bit too much. We're just a hobby project so it doesn't really matter that we break though. |
OK, understood. How about breaking changes to the methods of the type? The simplest starting point is to have these methods to create an Atom/String: fn new(s: &'a str) -> Atom<'a>;
fn new_in(s: &'a str, alloc: &'a Allocator) -> Atom<'a>;
// Limited to 16 bytes max
const fn new_const(s: &'static str) -> Atom<'static>; But I think in future it'd be better to remove fn new_from_source(SourceSlice<'a>) -> Atom<'a>;
So my question is: Is it a problem to do the initial implementation first, and then have the breaking change of removing |
Not a problem. We'll need to keep a change log from now on :-) |
OK great. I'm working on something else at the moment, but will come back to this soon. |
See comment on #2101:
If we can replace usage of Bumpalo's |
Previous comment turned out to be erroneous. But discovered a couple of other things along the way which are relevant to this issue:
|
…_allocator closes #1803 This `String` is currently unsafe, but I won't to get miri working before introducing more changes.
…_allocator closes #1803 This `String` is currently unsafe, but I won't to get miri working before introducing more changes.
…_allocator closes #1803 This `String` is currently unsafe, but I won't to get miri working before introducing more changes.
This PR partially fixes oxc-project#1803 and is part of oxc-project#1880. BigInt is removed from the `Token` value, so that the token size can be reduced once we removed all the variants. `Token` is now also `Copy`, which removes all the `clone` and `drop` calls. This yields 5% performance improvement for the parser.
…_allocator (oxc-project#2294) closes oxc-project#1803 This string is currently unsafe, but I want to get miri working before introducing more changes. I want to make a progress from memory leak to unsafe then to safety. It's harder to do the steps in one go.
OXC appears to leak memory when source includes any strings over 24 bytes in length.
A demonstration here: https://github.com/overlookmotel/oxc/tree/leak-test
Checkout that branch, and
cargo run -p test_leak
.The problem
I believe the problem is this:
Atom
s.Atom
wrapsCompactString
from compact_str.CompactString
allocates to the heap if string is longer than 24 bytes.Atom
s may appear anywhere in the AST, likely within aBox
.Box
used for the arena has noDrop
impl.Program
is dropped,Atom
s don't get dropped, and the heap allocationsCompactString
made are not freed.In the demo above, I've added
impl Drop for Atom
which logs when anAtom
is dropped. It never gets called, demonstrating that the heap memory is leaked.Possible solutions
1. Add
impl Drop
toBox
This would be like
bumpalo::collections::Box
.Problem is that this would make dropping the AST more expensive, negating a major benefit of using an arena.
2. Track and drop
Atom
s manuallyStore pointers to all non-inline
Atoms
as they're created. ADrop
impl on the arena could then drop them when the arena is dropped.3. Replace
CompactString
Replace
CompactString
with a similar "small string" implementation which stores long strings in the bumpalo arena, instead of the general heap.As well as solving the leak, I believe this would also be more performant:
CompactString
is quite complicated, but OXC only uses a small fraction ofCompactString
's capabilities, so creating a custom small string implementation in OXC I don't think would be too tricky.In addition:
CompactString
is built as a replacement for std'sString
, and includes acapacity
field to allow resizing. But OXC'sAtom
is immutable, so thecapacity
field is not required. All that's needed is aBox<str>
equivalent.So a new "small string" type could lose the
capacity
field and be reduced in size to 16 bytes, rather than the current 24. Obviously, only strings up to 16 bytes could then be stored inline, but still most JS identifiers are under 16 characters, and if string content is stored in the arena, storing longer strings "out of line" has less performance penalty.Further optimizations
For identifers without escapes (pretty much all identifiers) and string literals without escape sequences (most strings),
Atom
could just be a pointer to the string in source code, rather than copying the string into the arena.Secondly, when lexing strings and identifiers containing escape sequences,
AutoCow
creates a string in the arena. That string can be pointed to byAtom
rather than copied to another location in the arena.In fact, escaped strings for some reason currently are stored in the arena three times, though I'm not sure why. Dump of arena contents after parsing
x = 'ABCDE\n';
:The text was updated successfully, but these errors were encountered: