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

Arena allocator #2913

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Arena allocator #2913

wants to merge 8 commits into from

Conversation

kddnewton
Copy link
Collaborator

No description provided.

@kddnewton kddnewton changed the title allocator tmp Arena allocator WIP Jul 26, 2024
@kddnewton kddnewton changed the title Arena allocator WIP Arena allocator Jul 26, 2024
Copy link
Collaborator Author

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for a chance to rebase this

@Cluster444
Copy link

I'd love to help get this merged in! Working on a zig interface and this seems like it would make life easier.

I did a quick rebase, the conflicts were light, mostly around node_id which was new and the node_alloc function/macro pair.

The bigger part I'm guessing is around the use of xmalloc and friends since they don't take an allocator. Is the goal to replace xalloc/xfree with a call that takes an allocator?

@Cluster444
Copy link

Quick update, I did get this to rebase and all of the tests pass.

Though I had to kill xfree almost entirely as there's a few xfree calls that need to go away, just need to hunt those down.

@Cluster444
Copy link

So I think I have this sorted out, though I'm not sure how to get the results back to you here.

https://github.com/Cluster444/prism/tree/57b85f897d9e4c7ce60d725f0ff071fbbb59eae8

Here's the fork of this PR, rebased on main.

The only extra bit was lifting up all the field types that are being ignored in node destroy to make it a little more clear what those big lists were for.

@flavorjones
Copy link
Contributor

@Cluster444 I think it would be OK to create a new PR with your rebased branch.

@Cluster444 Cluster444 mentioned this pull request Nov 21, 2024
@kddnewton
Copy link
Collaborator Author

Mostly the blocker here is that we need to migrate CRuby over to this new API and ensure it still works, which is what I haven't had time to do yet.

@Cluster444
Copy link

It's looking like I have some free time tomorrow evening so I could have a look at that. I've got some basic experience with the CRuby internals, so I could start poking around and see what blows up. If you have any specific off-hand that come to mind that would help!

Also is the intention to get rid of most/all uses of heap allocs in favor of the arena? It seems mostly straightforward enough to do, but I also get the sense this is the first of a few optimization steps?

@kddnewton
Copy link
Collaborator Author

The goal is to get rid of the heap allocs that are done to allocate parts of the AST. So that freeing the AST doesn't require walking the tree. Basically the goal is to get rid of pm_node_destroy.

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.

4 participants