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

aligned allocation #8624

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from
Open

Conversation

DvirDukhan
Copy link
Contributor

This PR allows modules to use the allocator aligned allocation functionality via Redis modules api

@oranagra
Copy link
Member

oranagra commented Mar 9, 2021

@DvirDukhan please try to temporarily copy some of the other platforms CI from the daily.yml to ci.yml (just build is enough, no need to run the tests), so we can see that it builds on all of them.
@yossigo WDYT?

@yossigo
Copy link
Member

yossigo commented Mar 9, 2021

@oranagra What do you think about the other alternative of letting modules do their own custom allocations but have a way to report them so we can track it?

@oranagra
Copy link
Member

oranagra commented Mar 9, 2021

@yossigo i think there's value in both. some modules would like to report memory usage that's consumed by another allocator entirely, or one that they got by directly using mmap.
others may want to utilize redis's allocator but gain control of common allocator features (without worrying on which allocator is used by redis and what other things redis does when calling our allocator).

tests/modules/misc.c Outdated Show resolved Hide resolved
tests/modules/misc.c Outdated Show resolved Hide resolved
Comment on lines +142 to +147
#else
*((size_t*)ptr) = size;
update_zmalloc_stat_alloc(size+PREFIX_SIZE);
if (usable) *usable = size;
return (char*)ptr+PREFIX_SIZE;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how aligned allocation can work with no HAVE_MALLOC_SIZE, since by definition we're breaking alignment with this pointer manipulation.

Copy link
Member

Choose a reason for hiding this comment

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

they can work, if we modify the code to work around that (allocate a bit more extra).
i.e. add max(sizeof(size_t), alignment).

note that if the user uses his own allocator, he may have to keep the size somewhere too (there's no support for malloc_size), unless he knows the allocation size implicitly somehow (they're always the same size)

Copy link
Member

Choose a reason for hiding this comment

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

I guess alignment if often page sized, this means in practice we'd allocate size + page_size, just to use sizeof(size_t) bytes in the end of the previous page. I think this is quite a stretch...

Copy link
Member

Choose a reason for hiding this comment

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

i didn't consider such a big alignment request, is that common? i was thinking of 16 bytes and alike.
in that's common, we must fall back to your suggestion and let the module be able to append and deduct from our used_memory.

Copy link
Member

Choose a reason for hiding this comment

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

@DvirDukhan any input on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bringing this PR back from hibernate:
allowing aligned allocation for systems without HAVE_MALLOC_SIZE will cause problems with releasing the allocation, as there might be padding before the prefix.
so, reiterating the issue, we might need API for modules to expose update_zmalloc_stat_alloc and update_zmalloc_stat_free to modules
@MeirShpilraien what is your opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about adding a callback that will be called by Redis when it wants to know the currently used memory of the module (of memory that was allocated with an external allocator)? This way Redis can use it when calculating the currently used memory. I think this is a better approach because sometimes we might use libraries that do not allow us to override the default allocator but do allow us to get used to memory. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so in the case of a system call, like posix_memalign, the module has to maintain its own counters of the total memory allocated by allocations used outside of Redis, and it reports back to Redis. right?
seems reasonable

Copy link
Member

Choose a reason for hiding this comment

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

i don't think it's a good idea for redis to call a module callback every time it needs to know the memory usage.
it would have been ok if it was just in order to report it in INFO, but if redis needs to take this memory usage as if it was its own memory usage, there are tons of places that get the used memory, and they currently just read from a global variable, i don't want to replace that global variable read with a method with a loop.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @oranagra. I also don't think it's needed - well mannered libraries will anyway let you provide your own heap functions (which will map to module API heap functions).

@OfirMos
Copy link
Contributor

OfirMos commented Feb 7, 2022

@oranagra @yossigo @MeirShpilraien Please fix me if I'm wrong, from the quick research I've made the allocation size is used mostly for long time allocations (replication) and to cap memory.
If it was just to cap memory per module counter could have been used right?

The question: Is it possible to extern some allocation functions that don't save the size of the pointer for allocations which will be freed synchronously (for instance allocation for query execution).
Or maybe other solution will be to keep the allocation size on a designated struct?

The problems with the current solution is:

  1. for page sized allocations I'm afraid the size will cause performance and memory(overhead): it might cause the allocator to use larger slabs (for instance allocating 8k instead of 4k).
  2. for aligned allocations: it will also heart performance and memory consumption cause we need to allocate alignment(usually 16bytes) more than needed in order to assure alignment.

@oranagra
Copy link
Member

oranagra commented Feb 7, 2022

@OfirMos please note that for the vast majority of the target platforms, HAVE_MALLOC_SIZE is true, so redis doesn't allocate any additional bytes to count the memory, so if you allocate 4k, you only allocate 4k).

other notes:

  1. as you suggested, allocating temporary memory for execution of a query that is surely freed before it returns control to redis, will obviously not cause any issues if not counted in the global used_memory, but you will also be bypassing another mechanism (zmalloc_oom_handler).
  2. if you suggest to add an API for that as part of redis, i think that's "giving too much rope", i think it could be easily abused, and cause leaks and other. i don't mind if modules do funny things on their own, they're taking a risk, but adding such an API smells like recommending a bad approach.
  3. note that by using any other allocation function than the one redis provides, you risk using another allocator altogether, like redis will be using jemalloc and you'l be using libc malloc (p.s. same thing happens with the Lua VM today).
  4. the suggested solution in the link you provided is obviously bad (first because it's not portable, secondly because it's slow, and thirdly because it can not measure actually allocated memory, it can only measure VM-Size, and RSS).
  5. we could have queried jemalloc to ask how much memory it allocated, but that won't be portable either, and it's also slow (requires je_mallctl("epoch")

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants