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

Remove opj_aligned_malloc / opj_aligned_realloc / opj_aligned_free? #689

Closed
stweil opened this issue Jan 7, 2016 · 7 comments
Closed

Remove opj_aligned_malloc / opj_aligned_realloc / opj_aligned_free? #689

stweil opened this issue Jan 7, 2016 · 7 comments
Labels

Comments

@stweil
Copy link
Contributor

stweil commented Jan 7, 2016

Those functions are used to allocate memory which is aligned at a 16 byte boundary. For Linux, OpenJPEG also works without those special functions (same performance, same test results). The normal malloc / realloc functions already return aligned memory (8 byte boundary).

In a first step, src/lib/openjp2/opj_malloc.c could be simplified to map the three functions to their normal counterparts (opj_aligned_free => opj_free, ...).

In a second step, the three functions could be replaced in the whole code and removed from src/lib/openjp2/opj_malloc.c.

@malaterre
Copy link
Collaborator

no. the code is clear this way. you cannot assume anything particular about malloc()/free(). Using the aligned counterpart makes the use clear thoughrought the openjpeg code.

@malaterre malaterre reopened this Jan 7, 2016
@stweil
Copy link
Contributor Author

stweil commented Jan 7, 2016

@malaterre, you can assume a lot about malloc / free. :-) Especially you can assume that they return memory "which is suitably aligned for any built-in type" (cited from the Linux man page for malloc).

Larger alignment requirements are usually needed because of some hardware (like DMA controllers). This is not the case for OpenJPEG (as long as the code only runs on the normal CPU(s), not on some accelerator hardware).

Or are there any algorithms in OpenJPEG which assume a 16 byte alignment? Then a comment might be helpful.

@malaterre
Copy link
Collaborator

By "Linux man page for malloc", I assume you are talking about the default glibc implementation. Here is what I read: The GNU C Library:

The address of a block returned by malloc or realloc in the GNU system is always a multiple of eight (or sixteen on 64-bit systems). If you need a block whose address is a multiple of a higher power of two than that, use memalign, posix_memalign, or valloc. memalign is declared in malloc.h' and posix_memalign is declared instdlib.h'.

@malaterre
Copy link
Collaborator

BTW you were the initial report of an issue with an SSE instruction (cant remember from top of my head) that requires 16bytes aligned memory on some x86 machine (and it did segfault). So really I am totally missing the point here, and this is non-sense.

@stweil
Copy link
Contributor Author

stweil commented Jan 7, 2016

Thanks for the hint to SSE. I don't remember that I have reported a related problem, but I found several related issues in the list: #488, #624, #625.

Your notice on the alignment on 64-bit systems with the GNU C Library is also important: it explains why my 64-bit Debian GNU Linux does not need the aligned function variants - the default alignment is already 16 bytes.

Summary: The only known reason for special functions with 16 byte alignment is use of the x86 SSE instructions. Without SSE on x86 or on non x86 architectures, 16 byte alignment is either not needed or can theoretically even by insufficient.

@malaterre
Copy link
Collaborator

@stweil sorry I confused you with somebody else :(

Anyway the fact that C89 requires extension to provide specific alignement is an implementation detail. OpenJPEG is about providing a reference portable implementation of JPEG 2000. As such I believe that providing portable aligned allocators is a required step. Without such, there is no sense to start introducing restrict or assume keyword in the OpenJPEG code base to help optimizing compilers make the most of the C code. JPEG 2000 is written with such clever choice in mind, so I really would like to see those be implemented in OpenJPEG at some point.

Now if your request is about default compilation option, then that is a different story. And it does make sense to be able to compile OpenJPEG with a very strict C89 compiler which does not allow aligned alloctor, in which case OpenJPEG would still need to be conformant correct. If it does not, then I consider this a valid bug.

@rouault
Copy link
Collaborator

rouault commented Jul 5, 2017

It looks like this issue can be closed. opj_aligned_alloc_n() has a fallback to plain malloc() by playing witth offset to get the desired alignement

@rouault rouault closed this as completed Jul 5, 2017
@detonin detonin added the bug label Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants