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

Refactor JSMallocFunctions to simplify the implementation #525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Sep 11, 2024

Rather than having the user take care of JSMallocState, take care of the bookkeeping internally (and make JSMallocState non-public since it's no longer necessary) and keep the allocation functions to the bare minimum.

This has the advantage that using a different allocator is just a few lines of code, and there is no need to copy the default implementation just to moficy the call to the allocation function.

Fixes: #285

@saghul
Copy link
Contributor Author

saghul commented Sep 11, 2024

After looking at the alllocation stuff closely yesterday I remembered #285 and decided to give it a try.

I like the way it turned out and simplifies the user-land code, let me know what you think! @bnoordhuis @chqrlie

@saghul saghul force-pushed the refactor-malloc-funcs branch 3 times, most recently from ccca18c to 1762745 Compare September 12, 2024 07:23
@chqrlie
Copy link
Collaborator

chqrlie commented Sep 12, 2024

I like the way it turned out and simplifies the user-land code, let me know what you think! @bnoordhuis @chqrlie

Hi @saghul, sorry for the lag, a bit busy ATM, shall complete a review tonight :)

Rather than having the user take care of JSMallocState, take care of the
bookkeeping internally (and make JSMallocState non-public since it's no
longer necessary) and keep the allocation functions to the bare minimum.

This has the advantage that using a different allocator is just a few
lines of code, and there is no need to copy the default implementation
just to moficy the call to the allocation function.

Fixes: #285
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.

Overriding the memory allocator is needlessly complex
3 participants