Conversation
lum1n0us
force-pushed
the
feat/aligned-allocation
branch
from
a02ad8b to
6d08a77
Compare
Add public API for aligned memory allocation, exposing the existing mem_allocator_malloc_aligned infrastructure through wasm_export.h. - Add wasm_runtime_aligned_alloc() API declaration with documentation - Implement internal helper wasm_runtime_aligned_alloc_internal() - Add public function with size/alignment validation - POOL mode only, returns NULL for other memory modes - Follows wasm_runtime_malloc() patterns for consistency Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
lum1n0us
force-pushed
the
feat/aligned-allocation
branch
from
6d08a77 to
21857a9
Compare
TianlongLiang
left a comment
TianlongLiang
left a comment
There was a problem hiding this comment.
The first round review is on the source code. I will continue to review test cases
| return NULL; | ||
| } | ||
|
|
||
| if (size == 0) { |
There was a problem hiding this comment.
In the future, we may want to consider alternative implementations returning a NULL pointer when malloc(0) is called. Current impl is perfectly fine though, just some thoughts.
There was a problem hiding this comment.
Maybe a separate PR for be_queue?
| } | ||
|
|
||
| if (size > SIZE_MAX - alignment - HMU_SIZE - OBJ_PREFIX_SIZE | ||
| - OBJ_SUFFIX_SIZE - 8) { |
There was a problem hiding this comment.
What are these 8 bytes? If they are used to store those metadata
/* Store offset 8 bytes before returned pointer */
*((uint32_t *)((char *)ret - 8)) = offset;
/* Store magic with encoded alignment */
*((uint32_t *)((char *)ret - 4)) =
ALIGNED_ALLOC_MAGIC_VALUE | alignment_log2;Would you think it's better to use a Macro for those 8 bytes(like ALIGNED_HEADER or something) and modify a corresponding comment? Currently, the offset is not in it
* Memory Layout Diagram:
* ----------------------
*
* Low Address High Address
* ┌─────────────┬──────────┬────────────────┬──────────────┬─────────────┐
* │ HMU Header │ Padding │ Magic + Offset │ Aligned Data │ Padding │
* │ (meta) │ (0-align)│ (4 bytes) │ (size) │ (overhead) │
* └─────────────┴──────────┴────────────────┴──────────────┴─────────────┘
* ▲ ▲
* │ │
* magic_ptr user_ptr (returned, aligned)|
|
||
| /* Calculate total size needed */ | ||
| tot_size_unaligned = | ||
| HMU_SIZE + OBJ_PREFIX_SIZE + size + OBJ_SUFFIX_SIZE + alignment - 1 + 8; |
There was a problem hiding this comment.
Same question about 8 bytes here, and it seems that duplicate overflow check with L671-L673?
| if (!obj) | ||
| return false; | ||
|
|
||
| uint32_t *magic_ptr = (uint32_t *)((char *)obj - 4); |
There was a problem hiding this comment.
Also, maybe it's worth considering using a macro instead of hard code offset?
There was a problem hiding this comment.
Didn't see WASM_RUNTIME_API_INTERN be used in another place. Is it simply an indicator for testing purposes, like in the mem-alloc test? Or we would actually add those prefixes in the future to internal APIs?
|
|
||
| #include <stdarg.h> | ||
| #include <stddef.h> | ||
| #include <setjmp.h> |
There was a problem hiding this comment.
Do we need this header?
| assert_int_equal(hmu_get_ut(hmu_aligned), HMU_VO); | ||
|
|
||
| /* Sizes should be reasonable */ | ||
| assert_true(hmu_get_size(hmu_normal) >= 128); |
There was a problem hiding this comment.
Do we want to test the exact size? Or upper limit maybe
| } | ||
| } | ||
|
|
||
| assert_true(count > 10); /* At least some should succeed */ |
There was a problem hiding this comment.
Given the current size of heap_buf, I think maybe all should succeed
| } | ||
| } | ||
|
|
||
| assert_true(count > 20); |
There was a problem hiding this comment.
ditto, maybe we should be more aggressive?
2 participants
Please refer to core/shared/mem-alloc/ems/ems_gc_internal.h in PR to get more details about arch.