aws_customized_aligned_allocator #1147
Conversation
include/aws/common/allocator.h
Outdated
| * Create an aligned allocator with customized alignment. | ||
| */ | ||
| AWS_COMMON_API | ||
| struct aws_allocator *aws_customized_aligned_allocator_new( |
There was a problem hiding this comment.
maybe just aws_custom_alignment_allocator_new?
There was a problem hiding this comment.
I assume it's named this way because the nice simple name aws_aligned_allocator() is already taken, but that one automagically determines the alignment, and it differs based on allocation size.
Move these new functions to be right next to the existing aws_aligned_allocator() (instead of the end of the file) so that someone searching "align" is more likely to see both options and pick the right one.
I like "explicit" more than "custom". "custom" sounds like "you can do whatever you want like via a callback" while "explicit" sounds to me like "we simply use this 1 number you give us".
I'd suggest aws_explicit_aligned_allocator() (sounds like the pre-existing allocator) or aws_explicit_alignment_allocator() (nicer sounding gramatically)
source/allocator.c
Outdated
| struct aws_allocator *allocator; | ||
| size_t alignment_size; | ||
| }; | ||
|
|
||
| static void *s_aligned_malloc(struct aws_allocator *allocator, size_t size) { | ||
| (void)allocator; |
There was a problem hiding this comment.
probably not needed anymore?
source/allocator.c
Outdated
| */ | ||
| const size_t alignment = sizeof(void *) * (size > (size_t)PAGE_SIZE ? 8 : 2); | ||
| size_t alignment = sizeof(void *) * (size > (size_t)PAGE_SIZE ? 8 : 2); | ||
| if (allocator->impl) { |
There was a problem hiding this comment.
allocator will not have impl if its aligned by default?
There was a problem hiding this comment.
dumb idea, but maybe just let impl be the actual alignment (i.e. just cast impl pointer to size_t and back) instead of allocating another chunk of mem for it?
source/allocator.c
Outdated
| @@ -34,6 +34,11 @@ bool aws_allocator_is_valid(const struct aws_allocator *alloc) { | |||
| return alloc && AWS_OBJECT_PTR_IS_READABLE(alloc) && alloc->mem_acquire && alloc->mem_release; | |||
| } | |||
|
|
|||
| struct aws_aligned_allocator_impl { | |||
| struct aws_allocator *allocator; | |||
There was a problem hiding this comment.
it's really confusing to see code that's like allocator->allocator name this like underlying_allocator or parent_allocator or something
There was a problem hiding this comment.
oh wait, reading more code ... this allocator is only used to create this one single aws_aligned_allocator_impl struct? It's not actually used for the underlying allocations, those come from posix_memalign().
Maybe just don't even bother passing an allocator here, just use aws_default_allocator() to create the impl. That's what aws_mem_tracer_new() does.
When the user passes an allocator, there's a general understanding that THIS is the allocator that will be used for further allocations. But that's not the case here. If aligned-allocations leaks, the passed-in allocator has no idea, it wasn't involved at all.
include/aws/common/allocator.h
Outdated
| * Create an aligned allocator with customized alignment. | ||
| */ | ||
| AWS_COMMON_API | ||
| struct aws_allocator *aws_customized_aligned_allocator_new( |
There was a problem hiding this comment.
I assume it's named this way because the nice simple name aws_aligned_allocator() is already taken, but that one automagically determines the alignment, and it differs based on allocation size.
Move these new functions to be right next to the existing aws_aligned_allocator() (instead of the end of the file) so that someone searching "align" is more likely to see both options and pick the right one.
I like "explicit" more than "custom". "custom" sounds like "you can do whatever you want like via a callback" while "explicit" sounds to me like "we simply use this 1 number you give us".
I'd suggest aws_explicit_aligned_allocator() (sounds like the pre-existing allocator) or aws_explicit_alignment_allocator() (nicer sounding gramatically)
source/allocator.c
Outdated
| @@ -34,6 +34,11 @@ bool aws_allocator_is_valid(const struct aws_allocator *alloc) { | |||
| return alloc && AWS_OBJECT_PTR_IS_READABLE(alloc) && alloc->mem_acquire && alloc->mem_release; | |||
| } | |||
|
|
|||
| struct aws_aligned_allocator_impl { | |||
| struct aws_allocator *allocator; | |||
| size_t alignment_size; | |||
There was a problem hiding this comment.
trivial:
| size_t alignment_size; | |
| size_t alignment; |
source/allocator.c
Outdated
| @@ -34,6 +34,11 @@ bool aws_allocator_is_valid(const struct aws_allocator *alloc) { | |||
| return alloc && AWS_OBJECT_PTR_IS_READABLE(alloc) && alloc->mem_acquire && alloc->mem_release; | |||
| } | |||
|
|
|||
| struct aws_aligned_allocator_impl { | |||
There was a problem hiding this comment.
use "explicit" or "custom" in this struct name (match the public function name)
Otherwise, one would assume this is for aws_aligned_allocator()
Which is extra confusing because both share the s_aligned_malloc() call, and it's tough to explain "you can tell that it's NOT the aws_aligned_allocator() because it uses the aws_aligned_allocator_impl" 🤯🤯🤯
There was a problem hiding this comment.
OR make it so that both aws_aligned_allocator() and the new one BOTH use this struct. They both have an impl but member size_t explicit_alignment is 0 for aws_aligned_allocator().
then in s_aligned_malloc(), you can simply branch on whether or not impl->explicit_alignment is set
There was a problem hiding this comment.
Go with @DmitriyMusatkin 's suggestion to remove the whole impl, and use the point address to store the size_t for the alignment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1147 +/- ##
==========================================
+ Coverage 83.82% 83.86% +0.03%
==========================================
Files 57 57
Lines 5992 6005 +13
==========================================
+ Hits 5023 5036 +13
Misses 969 969 ☔ View full report in Codecov by Sentry. |
Issue #, if available:
aws_customized_aligned_allocatorto support customized aligned instead of the default one.Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.