aws_customized_aligned_allocator #1147

TingDaoK · 2024-08-22T23:58:50Z

Issue #, if available:

Add aws_customized_aligned_allocator to 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.

DmitriyMusatkin · 2024-08-23T00:08:10Z

include/aws/common/allocator.h

+ * Create an aligned allocator with customized alignment.
+ */
+AWS_COMMON_API
+struct aws_allocator *aws_customized_aligned_allocator_new(


maybe just aws_custom_alignment_allocator_new?

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)

DmitriyMusatkin · 2024-08-23T00:16:23Z

source/allocator.c

+    struct aws_allocator *allocator;
+    size_t alignment_size;
+};
+
 static void *s_aligned_malloc(struct aws_allocator *allocator, size_t size) {
    (void)allocator;


probably not needed anymore?

DmitriyMusatkin · 2024-08-23T00:20:50Z

source/allocator.c

     */
-    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) {


allocator will not have impl if its aligned by default?

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?

graebm · 2024-08-23T16:07:25Z

source/allocator.c

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


it's really confusing to see code that's like allocator->allocator name this like underlying_allocator or parent_allocator or something

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.

source/allocator.c

graebm · 2024-08-23T17:03:10Z

include/aws/common/allocator.h

+ * Create an aligned allocator with customized alignment.
+ */
+AWS_COMMON_API
+struct aws_allocator *aws_customized_aligned_allocator_new(


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)

graebm · 2024-08-23T17:09:02Z

source/allocator.c

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


trivial:

Suggested change

size_t alignment_size;

size_t alignment;

graebm · 2024-08-23T17:18:20Z

source/allocator.c

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


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" 🤯🤯🤯

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

Go with @DmitriyMusatkin 's suggestion to remove the whole impl, and use the point address to store the size_t for the alignment.

codecov-commenter · 2024-08-23T18:22:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.86%. Comparing base (88f0ea3) to head (095b34f).

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.
📢 Have feedback on the report? Share it here.

TingDaoK added 3 commits August 22, 2024 16:47

add a customized alignment allocator

dd4e827

add test

c4d45ea

copy.paste

9e81964

DmitriyMusatkin reviewed Aug 23, 2024

View reviewed changes

graebm reviewed Aug 23, 2024

View reviewed changes

TingDaoK added 3 commits August 23, 2024 11:09

address comments

f888d22

0 is not a valid alignment and fix

be99adf

ops. missing export

b91b75b

Merge branch 'main' into align-allocator-size

095b34f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws_customized_aligned_allocator #1147

aws_customized_aligned_allocator #1147

TingDaoK commented Aug 22, 2024

DmitriyMusatkin Aug 23, 2024

graebm Aug 23, 2024

DmitriyMusatkin Aug 23, 2024

DmitriyMusatkin Aug 23, 2024

DmitriyMusatkin Aug 23, 2024

graebm Aug 23, 2024

graebm Aug 23, 2024

graebm Aug 23, 2024

graebm Aug 23, 2024

graebm Aug 23, 2024

graebm Aug 23, 2024 •

edited

Loading

TingDaoK Aug 23, 2024

codecov-commenter commented Aug 23, 2024 •

edited

Loading

aws_customized_aligned_allocator #1147

Are you sure you want to change the base?

aws_customized_aligned_allocator #1147

Conversation

TingDaoK commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graebm Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 23, 2024 • edited Loading

Codecov Report

graebm Aug 23, 2024 •

edited

Loading

codecov-commenter commented Aug 23, 2024 •

edited

Loading