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

Removed unused alignment/padding calculation from zmalloc macros #4770

Conversation

jonahharris
Copy link
Contributor

@jonahharris jonahharris commented Mar 19, 2018

The alignment/padding calculations in update_zmalloc_stat_(alloc|free) currently do nothing, haven't been used since thread safe mode was removed, and were probably broken years before that as they were used only for incrementation in the thread safe case. As the padding is accounted for in all calls to update_zmalloc_stat_(alloc|free) by the caller either using zmalloc_size or explicit addition of PREFIX_SIZE, they no longer appear to be needed here.

See issue #4739

@wkgcass
Copy link

wkgcass commented May 12, 2018

I check the commit which is 3 years before now, both thread_safe and non_thread_safe mode use the variable _n.
The commit that changes _n to __n was c7b46a4.
Consider that malloc almost always allocates the memory with alignment padding, so I think the fix would be using the _n, not removing the padding calculation.

However I personally support your solution.
We never know how much bytes exactly that one malloc call takes, so assuming 8 bytes alignment is not always correct. Besides, the memory used to store memory info is not taken into account (I'm not talking about PREFIX_SIZE, but the memory spaces before malloc returned pointer). The result is that used_memory is never correct, always smaller than it actually is.
In the situation of macos (which uses macos allocator), malloc_size result is also not exactly how much the process consumes.
So my point is that, since we cannot know how much exactly we consumed, why not just ignore that alignment, and the used_memory statistics tells us how much memory the process at most required.

I am using zmalloc to count used memory in my project, and I changed the behavior of update_zmalloc_stat_alloc|free and zmalloc_size, to count or return exactly the number when calls malloc (i.e. size+PREFIX_SIZE).

commit is here (wkgcass/nanovm@e58f67f)

I suggest redis do the same. But looking back to the commit history, maybe maintainers would like to keep the alignment.

@antirez
Copy link
Contributor

antirez commented May 31, 2018

Marked for review before RC2. Thanks!

@oranagra
Copy link
Member

@jonahharris sorry i wasn't aware of this PR, and already fixed it in 50f5181
thanks.

@oranagra oranagra closed this Aug 28, 2020
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

4 participants