Skip to content

add a pm_arena_reset API method#3986

Closed
froydnj wants to merge 1 commit intoruby:mainfrom
froydnj:froydnj-arena-reset
Closed

add a pm_arena_reset API method#3986
froydnj wants to merge 1 commit intoruby:mainfrom
froydnj:froydnj-arena-reset

Conversation

@froydnj
Copy link
Contributor

@froydnj froydnj commented Mar 11, 2026

This PR is half commit-as-is, half starting point for discussion.

We have some Rust analysis programs that parse a bunch of Ruby files in one go. The way things are currently structured, the arena used for one file and parse needs to be completely freed, and the next file needs to start reallocating blocks out of a completely fresh arena. These new blocks may or may not be the same blocks, and if the files are sufficiently large, we need to repeatedly allocate multiple blocks for each file. I think the same thing is true for something like Sorbet parsing a bunch of files in one go. But pm_arena_t doesn't offer any API functions that would let you reset the state of the allocator.

It seems like it'd be useful to try and re-use arenas between parsing calls: we could immediately start reusing the biggest chunk of memory allocated thus far, and we don't need to waste time repeatedly allocating blocks of memory and freeing them. Hence this PR to add pm_arena_reset to almost, but not quite, completely reset the arena so it can be reused.

I know of at least one precedent for this kind of API: the bumpalo Rust crate offers Bump#reset which does exactly the same thing offered here. GNU's obstacks are not quite the same thing as arena allocation, but they do allow you to reuse an obstack in roughly the same way. I imagine there are other instances, but those are the two I know about.

Full disclosure: I have no stats backing it up that this would actually be faster or be so much faster that it's worthwhile having the extra code in Prism.

This PR would either require followon PRs to figure out what the Rust-side API would look like (e.g. do we need/want to PRISM_EXPORTED_FUNCTION pm_arena_reset? how does the ruby_prism::parse method need to change, or what does an additional API look like? etc.), or would require some iteration, assuming that it's accepted in the first place.

@froydnj froydnj force-pushed the froydnj-arena-reset branch from 36a6296 to 4ecee01 Compare March 11, 2026 13:54
@kddnewton
Copy link
Collaborator

So I considered adding this API when I first added the arena, but honestly I could not get it to make a big enough difference to justify it.

With the approach you have in the PR I don't think it will help because the last block is not going to be big enough. If you wanted to the take approach of count up the used memory, then if the used memory fits into the last block then free everything except the last block and return, otherwise free everything and allocate a block with the sum of the previous blocks, I found that approach yields about a 3% speed win on parsing everything in ruby/ruby while retaining more memory. But I'm not sure if it's worth it.

@froydnj
Copy link
Contributor Author

froydnj commented Mar 11, 2026

So I considered adding this API when I first added the arena, but honestly I could not get it to make a big enough difference to justify it.

That's helpful context, thank you.

With the approach you have in the PR I don't think it will help because the last block is not going to be big enough. If you wanted to the take approach of count up the used memory, then if the used memory fits into the last block then free everything except the last block and return, otherwise free everything and allocate a block with the sum of the previous blocks, I found that approach yields about a 3% speed win on parsing everything in ruby/ruby while retaining more memory. But I'm not sure if it's worth it.

Ah, I see, you're not doubling the size of the blocks on every allocation, but every N allocations or so. OK, yeah, you'd have to do a block reallocation strategy here.

I'd be curious whether a straight doubling strategy (or even multiplying by 3/2) would be better overall, with or without this PR.

In any event, I'm happy to close this.

@froydnj froydnj closed this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants