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

aws_customized_aligned_allocator #1147

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions include/aws/common/allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,21 @@ size_t aws_small_block_allocator_page_size(struct aws_allocator *sba_allocator);
AWS_COMMON_API
size_t aws_small_block_allocator_page_size_available(struct aws_allocator *sba_allocator);

/*
* Create an aligned allocator with customized alignment.
*/
AWS_COMMON_API
struct aws_allocator *aws_customized_aligned_allocator_new(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just aws_custom_alignment_allocator_new?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

struct aws_allocator *allocator,
size_t customized_alignment);

/*
* Destroys a customized aligned allocator instance and frees its memory to the parent allocator. The parent
* allocator will otherwise be unaffected.
*/
AWS_COMMON_API
void aws_customized_aligned_allocator_destroy(struct aws_allocator *aligned_alloc);

AWS_EXTERN_C_END
AWS_POP_SANE_WARNING_LEVEL

Expand Down
58 changes: 56 additions & 2 deletions source/allocator.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@graebm graebm Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

struct aws_allocator *allocator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

size_t alignment_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trivial:

Suggested change
size_t alignment_size;
size_t alignment;

};

static void *s_aligned_malloc(struct aws_allocator *allocator, size_t size) {
(void)allocator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not needed anymore?

/* larger allocations should be aligned so that AVX and friends can avoid
Expand All @@ -46,9 +51,15 @@ static void *s_aligned_malloc(struct aws_allocator *allocator, size_t size) {
* 8 byte alignment for <= page allocations on 32 bit systems
*
* We use PAGE_SIZE as the boundary because we are not aware of any allocations of
* this size or greater that are not data buffers
* this size or greater that are not data buffers.
*
* Unless there is a customized alignment size.
*/
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allocator will not have impl if its aligned by default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

struct aws_aligned_allocator_impl *impl = allocator->impl;
alignment = impl->alignment_size;
}
TingDaoK marked this conversation as resolved.
Show resolved Hide resolved
#if !defined(_WIN32)
void *result = NULL;
int err = posix_memalign(&result, alignment, size);
Expand Down Expand Up @@ -168,6 +179,49 @@ struct aws_allocator *aws_aligned_allocator(void) {
return &aligned_allocator;
}

struct aws_allocator *aws_customized_aligned_allocator_new(
struct aws_allocator *allocator,
size_t customized_alignment) {
if ((customized_alignment & (customized_alignment - 1)) != 0 || customized_alignment % sizeof(void *) != 0) {
/**
* the alignment must be a power of two and a multiple of sizeof(void *)
*/
aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
return NULL;
}

struct aws_aligned_allocator_impl *aligned_alloc_impl = NULL;
struct aws_allocator *aligned_alloc = NULL;
aws_mem_acquire_many(
allocator,
2,
&aligned_alloc_impl,
sizeof(struct aws_aligned_allocator_impl),
&aligned_alloc,
sizeof(struct aws_allocator));

AWS_ZERO_STRUCT(*aligned_alloc_impl);
AWS_ZERO_STRUCT(*aligned_alloc);
/* copy the template vtable */
*aligned_alloc = aligned_allocator;
aligned_alloc->impl = aligned_alloc_impl;
aligned_alloc_impl->alignment_size = customized_alignment;
aligned_alloc_impl->allocator = allocator;
return aligned_alloc;
}

void aws_customized_aligned_allocator_destroy(struct aws_allocator *aligned_alloc) {
if (!aligned_alloc) {
return;
}
struct aws_aligned_allocator_impl *impl = aligned_alloc->impl;
if (!impl) {
return;
}
struct aws_allocator *allocator = impl->allocator;
aws_mem_release(allocator, impl);
}

void *aws_mem_acquire(struct aws_allocator *allocator, size_t size) {
AWS_FATAL_PRECONDITION(allocator != NULL);
AWS_FATAL_PRECONDITION(allocator->mem_acquire != NULL);
Expand Down
3 changes: 3 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ add_test_case(default_threaded_reallocs)
add_test_case(default_threaded_allocs_and_frees)
add_test_case(aligned_threaded_reallocs)
add_test_case(aligned_threaded_allocs_and_frees)
add_test_case(customized_aligned_sanitize)
add_test_case(customized_aligned_threaded_reallocs)
add_test_case(customized_aligned_threaded_allocs_and_frees)

add_test_case(test_memtrace_none)
add_test_case(test_memtrace_count)
Expand Down
58 changes: 57 additions & 1 deletion tests/alloc_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ static int s_default_threaded_allocs_and_frees(struct aws_allocator *allocator,
AWS_TEST_CASE(default_threaded_allocs_and_frees, s_default_threaded_allocs_and_frees)

/*
* No align allocator tests.
* aligned allocator tests.
*/
static int s_aligned_threaded_reallocs(struct aws_allocator *allocator, void *ctx) {
(void)allocator;
Expand Down Expand Up @@ -381,3 +381,59 @@ static int s_aligned_threaded_allocs_and_frees(struct aws_allocator *allocator,
return 0;
}
AWS_TEST_CASE(aligned_threaded_allocs_and_frees, s_aligned_threaded_allocs_and_frees)

static int s_customized_aligned_sanitize(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

struct aws_allocator *aligned_alloc = aws_customized_aligned_allocator_new(allocator, 1);
ASSERT_NULL(aligned_alloc);
ASSERT_UINT_EQUALS(aws_last_error(), AWS_ERROR_INVALID_ARGUMENT);

aligned_alloc = aws_customized_aligned_allocator_new(allocator, 3 * sizeof(void *));
ASSERT_NULL(aligned_alloc);
ASSERT_UINT_EQUALS(aws_last_error(), AWS_ERROR_INVALID_ARGUMENT);

size_t aligned_size = 1024;
aligned_alloc = aws_customized_aligned_allocator_new(allocator, aligned_size);
ASSERT_NOT_NULL(aligned_alloc);
void *test = aws_mem_acquire(aligned_alloc, sizeof(void *));
ASSERT_TRUE((uintptr_t)test % aligned_size == 0);
aws_mem_release(aligned_alloc, test);

aws_customized_aligned_allocator_destroy(aligned_alloc);

return 0;
}
AWS_TEST_CASE(customized_aligned_sanitize, s_customized_aligned_sanitize)

static int s_customized_aligned_threaded_reallocs(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
srand(15);
struct aws_allocator *aligned_alloc = aws_customized_aligned_allocator_new(allocator, 512);

struct aws_allocator *alloc = aws_mem_tracer_new(aligned_alloc, NULL, AWS_MEMTRACE_STACKS, 8);

s_thread_test(alloc, s_threaded_realloc_worker, alloc);

aws_mem_tracer_destroy(alloc);
aws_customized_aligned_allocator_destroy(aligned_alloc);

return 0;
}
AWS_TEST_CASE(customized_aligned_threaded_reallocs, s_customized_aligned_threaded_reallocs)

static int s_customized_aligned_threaded_allocs_and_frees(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
srand(99);
struct aws_allocator *aligned_alloc = aws_customized_aligned_allocator_new(allocator, 512);

struct aws_allocator *alloc = aws_mem_tracer_new(aligned_alloc, NULL, AWS_MEMTRACE_STACKS, 8);

s_thread_test(alloc, s_threaded_alloc_worker, alloc);

aws_mem_tracer_destroy(alloc);
aws_customized_aligned_allocator_destroy(aligned_alloc);

return 0;
}
AWS_TEST_CASE(customized_aligned_threaded_allocs_and_frees, s_customized_aligned_threaded_allocs_and_frees)
Loading