Skip to content

add yp_alloc to begin creating our own allocator#1375

Closed
HParker wants to merge 1 commit intoruby:mainfrom
HParker:add-yp_alloc
Closed

add yp_alloc to begin creating our own allocator#1375
HParker wants to merge 1 commit intoruby:mainfrom
HParker:add-yp_alloc

Conversation

@HParker
Copy link
Copy Markdown
Collaborator

@HParker HParker commented Sep 1, 2023

This introduces yp_malloc yp_calloc and yp_free as allocation APIs which we can use to manage allocations.

Once these are centralized we can use xmalloc if in a Ruby context and/or use an arena allocator to reduce syscalls to malloc.

Related to this issue: #1337

This is a small part of my working arena allocator here: https://github.com/HParker/yarp/tree/introduce-yp_malloc

In an effort to make smaller changes at a time, this only updates direct malloc/calloc/free calls in yarp.c. There are more allocations that I will move in followup PRs.

Copy link
Copy Markdown
Collaborator

@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.

In general this looks good, but before we merge I'd like to see the functions in yp_alloc.c marked as static that shouldn't be visible to other translation units. Also, we definitely need some comments on all of the functions that are visible beyond yp_alloc.h.

I'm not entirely clear on the USE_ARENA macro. It looks like it's triggering behavior, but it's always set to 1. Is this meant as a migration path? If so I would think it should be set to 0.

@HParker
Copy link
Copy Markdown
Collaborator Author

HParker commented Sep 6, 2023

Thanks @kddnewton I incorporated your feedback.

I removed the USE_ARENA macro, was there to make comparing performance easier, but that doesn't need to be here now, so I removed it.

How does this look now?

@HParker
Copy link
Copy Markdown
Collaborator Author

HParker commented Sep 8, 2023

Closing while we discuss direction to reopen later.

@HParker HParker closed this Sep 8, 2023
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