Stub implementation of new custom memory API #3437
Open
+52
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stedolan
force-pushed
the
stub-custom-alloc-dep
branch
from
93e52c4 to
90f16ac
Compare
stedolan
force-pushed
the
stub-custom-alloc-dep
branch
from
90f16ac to
4f574f6
Compare
NickBarnes
approved these changes
Jan 13, 2025
There was a problem hiding this comment.
Broadly this is OK, and we should probably merge it as-is. Three minor points which we could address when we put the implementation in:
- I'm unclear why
bigarray.cis so different between runtime 4 and runtime 5; - I would find this stuff easier to navigate with a clearer distinction in variable names and comments between custom block sizes and dependent memory sizes. Maybe consistently using
dep_size? - Moar comments plzkthxbai.
| reclaimed. | ||
| */ | ||
| CAMLextern value caml_alloc_custom_dep(struct custom_operations * ops, | ||
| uintnat size, /*size in bytes*/ |
There was a problem hiding this comment.
I'd like a clearer distinction in the comments between size and mem.
| @@ -251,7 +251,7 @@ caml_ba_alloc(int flags, int num_dims, void * data, intnat * dim) | |||
| uses_resources = | |||
| ((flags & CAML_BA_MANAGED_MASK) == CAML_BA_MANAGED) | |||
| && !(flags & CAML_BA_SUBARRAY); | |||
| res = caml_alloc_custom_mem(&caml_ba_ops, asize, uses_resources ? size : 0); | |||
| res = caml_alloc_custom_dep(&caml_ba_ops, asize, uses_resources ? size : 0); | |||
There was a problem hiding this comment.
Unclear to me why this is different in runtime4.
| CAMLexport value caml_alloc_custom_dep(const struct custom_operations * ops, | ||
| uintnat size, mlsize_t mem) | ||
| { | ||
| return caml_alloc_custom_mem(ops, size, mem); |
There was a problem hiding this comment.
Add a comment indicating what else will go in here in future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a stub implementation of @damiendoligez's new custom memory allocation API. The new API tracks both allocation and deallocation of dependent memory using
caml_alloc_dependent_mem / caml_free_dependent_mem, and provides a convenience functioncaml_alloc_custom_depwhich combines a custom block allocation and a call tocaml_alloc_dependent_mem.The difference from
caml_alloc_custom_memis that a user ofcaml_alloc_custom_depmust be sure to callcaml_free_depedent_memwhen the custom block is finalised.In this PR, the new API is added so that we can port code to it, but under the hood it just calls
caml_alloc_custom_memand thealloc_dependent/free_dependentfunctions are no-ops.(The changes to the existing API functions
caml_alloc/free_dependent_memare not backwards compatible. However, I'm not aware of any actual users of these functions, and if we find one someday it's easy to adapt)