port: Use memalign or _aligned_malloc only, when available, fallback to malloc.#1863
port: Use memalign or _aligned_malloc only, when available, fallback to malloc.#1863Dead2 merged 1 commit intozlib-ng:developfrom
Conversation
WalkthroughThe pull request enhances the build configuration and memory management code by adding checks for various aligned memory allocation functions (including Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| endif() | ||
| set(CMAKE_REQUIRED_DEFINITIONS) | ||
|
|
||
| check_symbol_exists(_aligned_malloc malloc.h HAVE__ALIGNED_MALLOC) |
There was a problem hiding this comment.
Two underscores. Is that intentional?
There was a problem hiding this comment.
Yes, the function starts with an underscore.
Without the second underscore, the difference between the
C11 aligned allocation macro and the
MS aligned allocation macro is only one character
and can easily mixed:
(HAVE_ALIGNED_ALLOC and HAVE_ALLIGNED_MALLOC) compared to
(HAVE_ALIGNED_ALLOC and HAVE__ALLIGNED_MALLOC)
But of course, when the project wants macros with only one underscore, please let me know.
|
When a similar check for cpuid is added, OpenWatcom can build "zlib-ng": Some TODO items for OpenWatcom:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1863 +/- ##
============================================
+ Coverage 42.10% 79.30% +37.20%
============================================
Files 71 132 +61
Lines 7353 12199 +4846
Branches 1265 2684 +1419
============================================
+ Hits 3096 9675 +6579
+ Misses 4035 1538 -2497
- Partials 222 986 +764 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| void *ptr; | ||
| return posix_memalign(&ptr, 64, size) ? NULL : ptr; | ||
| #elif defined(_WIN32) | ||
| #elif defined(HAVE__ALIGNED_MALLOC) |
There was a problem hiding this comment.
It might be worth moving this to the top just in case there's a platform that supports both _aligned_malloc and aligned_alloc/posix_memalign, to make sure that the correct free function is used.
There was a problem hiding this comment.
Oh yeah.
I changed the detection strategy
for this Revision to reduce indention
(Posix and c11.
Check gnu ext. only, when c11 ist absent
Check MS ext only, when gnu ext. ist absent)
That's better to read, but i forgot to handle the free function.
I will prepare an updated Patch.
zutil_p.h
Outdated
| return (void *)memalign(64, size); | ||
| #else | ||
| /* Fallback to use a normal allocation and hope the best */ | ||
| /* This works only, when the default C compression code is used */ |
There was a problem hiding this comment.
From what I can tell, zlib-ng does ensure that allocated memory is aligned at a higher level, so it might be worth clarifying that in the comments.
There was a problem hiding this comment.
I know only about the alignments required for SSE/AVX.
There was a problem hiding this comment.
What I meant is that zlib-ng currently includes padding when allocating, which means it can make sure that the buffer is aligned even if the allocation callback doesn't guarantee it (since it might be provided by users of the library rather than using the default implementation).
https://github.com/zlib-ng/zlib-ng/blob/develop/deflate.c#L246
https://github.com/zlib-ng/zlib-ng/blob/develop/inflate.c#L181
|
Needs rebase, fail with |
| @@ -10,8 +10,11 @@ | |||
| #elif defined(__FreeBSD__) | |||
There was a problem hiding this comment.
nitpick (non-blocking): Since you removed the APPLE below in block choosing malloc type in favor of using a fallback, perhaps also remove the above check for APPLE as well.
You could also remove the whole first check there, and just add a comment in the fallback that HAVE_POSIX_MEMALIGN and HAVE_ALIGNED_ALLOC as well as standard malloc are all handled by the fallback. Seems redundant that both the first and the last block are the same here.
There was a problem hiding this comment.
Ok.
I will prepare a follow-up patch to clean it up
after this change is committed.
zutil_p.h
Outdated
| #elif defined(HAVE_MEMALIGN) || defined(HAVE__ALIGNED_MALLOC) | ||
| # include <malloc.h> | ||
| #else | ||
| /* Fallback, when no other aligned allocation was found */ |
There was a problem hiding this comment.
Nitpick: Maybe "aligned allocation function" would be better...
…alloc Using "_WIN32" to decide, if the MSVC extensions _aligned_malloc / _aligned_free are available is a bug that breaks other Compiler on Windows. (OpenWatcom as Example) Regards ... Detlef
|
IMO, better not to bring this messy complexity and do it ffmpeg/x264 way: they completely abandoned these and simply align memory themselves. Not sure who of them does, or maybe both. If the code needs aligned memory and there is no routines you need to fallback to manual alignment: could as well keep using it for all to make it simple. cost of manual alignment is zero. malloc64:
uint8_t *buf = (uint8_t*)malloc(size + 63 + sizeof(void**));
if (buf)
{
uint8_t *aligned = buf + 63 + sizeof(void**);
aligned -= (intptr_t)aligned & 63;
*((void**)(aligned- sizeof(void **))) = buf;
return aligned;
}
free64:
if(p)
free( *(((void**)p) - 1));something like that |
|
@pps83 Unless I missed something, and I don't think I did, all allocs should already get checked for alignment and if not already aligned it will manually align them. See #1713. The reason we always check for alignment is that the zlib api allows applications giving it pointers to its preferred alloc/free functions, and we cannot know whether those will supply us with aligned memory. |
This fixes also the usage of "_WIN32" to decide,
if the MSVC extensions _aligned_malloc / _aligned_free are available
That bug breaks other Compiler on Windows. (OpenWatcom as Example).
--
Regards ... Detlef
Summary by CodeRabbit
New Features
Refactor