aligned memory allocaion #4277

LeonidGoltsblat · 2025-01-29T14:17:31Z

Currently pjsip library supports only global (library level) memory allocation alignment control.
This PR introduce more granular control of this process.

PJ_IDECL(pj_pool_t*) pj_pool_aligned_create(pj_pool_factory *factory,
                                            const char *name,
                                            pj_size_t initial_size,
                                            pj_size_t increment_size,
                                            pj_size_t alignment,
                                            pj_pool_callback *callback);

This feature allows to set the default alignment for all subsequent allocations from the pool created here.
The actual allocation alignment will be at least equal to the alignment argument of the function, but not less than PJ_POOL_ALIGNMENT. (Pool created by pj_pool_create() has the default alignment equal to PJ_POOL_ALIGNMENT.)

The second API allow to control the alignment of memory allocated by the current allocation request:

PJ_IDECL(void *) pj_pool_aligned_alloc(pj_pool_t *pool, pj_size_t alignment,
                                       pj_size_t size);

The actual alignment of the allocation will be at least equal to the alignment argument of the function, but not less than the default pool alignment specified when the pool was created.

bennylp

Thanks for the patch. You beat me to it! Pls find my review, sorry there are many of them!

Plus pls find patch to make PJ_HAS_POOL_ALT_API work (at least compilable):
alt_pool_diff.txt

bennylp · 2025-01-30T07:18:10Z

pjlib/include/pj/pool_i.h

@@ -20,6 +20,8 @@

 #include <pj/string.h>

+#define ALIGN_PTR(PTR,ALIGNMENT)    (PTR + (-(pj_ssize_t)(PTR) & (ALIGNMENT-1)))


Rename to PJ_POOL_ALIGN_PTR, since this potentially be included as public API

#define IS_ALIGNED(p, a) (!((uintptr_t)(p) & ((a)-1)))
this is from row.h libyuv

Shouldn't this be public too?

The objective here is to prevent conflict from symbols that we (pjsip) declare with symbols from other software when the users include our header files mixed with header files from other software. That's why we put pj/PJ prefix everywhere.

SInce pool_i.h can potentially be included by user (i.e. when PJ_FUNCTIONS_ARE_INLINED is set to nonzero), all symbols declared here must be properly prefixed with pj/PJ.

As for IS_ALIGNED in libyuv/row.h, since this is not our software (although it is included in our distribution), we can't control what naming they use, and we don't want to modify it, so it is what it is.

bennylp · 2025-01-30T07:20:00Z

pjlib/include/pj/pool_i.h

-    if ((pj_size_t)(block->end - block->cur) >= size) {
-        void *ptr = block->cur;
-        block->cur += size;
+    unsigned char *ptr = ALIGN_PTR(block->cur, alignment);


Pls move ptr declaration to before first statement (re: coding style)

bennylp · 2025-01-30T07:30:58Z

pjlib/include/pj/pool_i.h

@@ -37,28 +39,38 @@ PJ_IDEF(pj_size_t) pj_pool_get_used_size( pj_pool_t *pool )
    return used_size;
 }

-PJ_IDEF(void*) pj_pool_alloc_from_block( pj_pool_block *block, pj_size_t size )
+PJ_IDEF(void*) pj_pool_alloc_from_block( pj_pool_block *block, pj_size_t alignment,
+                                         pj_size_t size )
 {


I think we need to check (PJ_ASSERT_RETURN) either here or in pj_pool_aligned_alloc() that the value of alignment is really power of two. Sample implementation:

#define IS_POWER_OF_TWO(val) (((val)>0) && ((val) & ((val)-1))==0)

Also in pj_pool_aligned_create(), pj_pool_allocate_find(), and possibly others.

NB!
/* The operation below is valid for size==0.
* When size==0, the function will return the pointer to the pool
* memory address, but no memory will be allocated.
*/

c++ new() returns uniqiue address for each allocation (even for size==0). This may be better than pjsip's behavior.

IMO the existing spec is okay, it can be used for example to check the pointer belongs to some buffer.

bennylp · 2025-01-30T07:41:48Z

pjlib/include/pj/pool_i.h

+{
+    if (alignment < pool->alignment)
+        alignment = pool->alignment;
+    void *ptr = pj_pool_alloc_from_block(pool->block_list.next, 


Pls move ptr declaration to before first statement (re: coding style)

bennylp · 2025-01-30T07:58:41Z

pjlib/src/pj/pool_caching.c

 {
    pj_caching_pool *cp = (pj_caching_pool*)pf;
    pj_pool_t *pool;
    int idx;

    PJ_CHECK_STACK();
+    PJ_UNUSED_ARG(alignment);


This shouldn't be necessary since alignment is used

bennylp · 2025-01-30T08:53:24Z

pjlib/src/pjlib-test/pool.c

+        if (i % 2)
+        {
+            ptr = pj_pool_aligned_alloc(pool, POOL_ALIGNMENT_TEST, 1);
+            if (!IS_ALIGNED2(ptr))


While we're at it, I would like to introduce you to the new unit test API (pj/unittest.h). :)
This block:

if (!IS_ALIGNED2(ptr)) { pj_pool_release(pool); pj_pool_release(pool2); return -312; }

can be replaced with:

PJ_TEST_TRUE(IS_ALIGNED2(ptr), NULL, {rc=-312; goto on_return; });

and replace this at the end of the function:

/* Done */ pj_pool_release(pool); pj_pool_release(pool2); return 0;

with:

/* Done */ on_return: pj_pool_release(pool); pj_pool_release(pool2); return rc;

Plus declare and initialize rc at the start of the function, and #define THIS_FILE "pool.c" in the file (needed by unit test macros).

Done in my PR (in your repo)

bennylp · 2025-01-30T08:54:36Z

pjlib/src/pjlib-test/pool.c

+            {
+                pj_pool_release(pool);
+                pj_pool_release(pool2);
+                return -312;


Duplicate error value. Also consider re-sorting them, and give some space between the values (so that if we insert new test item, there is no need to replace other return values).

Done in my PR (in your repo)

bennylp · 2025-01-30T09:39:41Z

pjlib/src/pj/pool.c

    {
        pj_size_t count;
        count = (size + pool->increment_size + sizeof(pj_pool_block) +
-                 PJ_POOL_ALIGNMENT) / 
+                 alignment) /


same bug as above

Looks like GH losts my long written comment... :(
It's supposed to be commenting line 134. I think there is a bug, it's not your bug but it's an existing bug. I think it should be added by 2*alignment instead of just one alignment. The rationale is, in worst case, the alignment is done twice. First to align start of buffer, then to align the size itself.

I can reproduce the bug with this code (it's producing assertion error in pj/pool.c:157):

int init_size = 50 + sizeof(pj_pool_t) + sizeof(pj_pool_block); int pool_alignment = 4; int huge_alignment = 256; int inc_size = 50; void *ptr; pj_pool_t *pool = pj_pool_aligned_create(mem, NULL, init_size, inc_size, pool_alignment, &null_callback); ptr = pj_pool_aligned_alloc(pool, huge_alignment, 1); pj_pool_release(pool); PJ_TEST_NOT_NULL(ptr, NULL, return -220);

If you don't mind you can add that to the pool's unit test

bennylp · 2025-01-30T10:13:09Z

Note: the API compatibility is broken (potentially) for pool implementors and not pool users.

LeonidGoltsblat · 2025-01-30T21:25:02Z

Hi! I'm currently "out of my virtual office" and will be back early next week.

Regarding alt_pool_diff.txt: is it possible to find this commit somewhere on GH? Or can you push it to my fork, please?

bennylp · 2025-01-30T23:39:08Z

Hi! I'm currently "out of my virtual office" and will be back early next week.

No worries.

Regarding alt_pool_diff.txt: is it possible to find this commit somewhere on GH? Or can you push it to my fork, please?

Done, created PR in your fork. I probably can create PR for the other fixes too.

aligned memory allocaion

3e7fedf

sauwming requested review from bennylp, sauwming and nanangizz January 30, 2025 00:51

sauwming added type: enhancement component: pjlib labels Jan 30, 2025

bennylp added the note:api-break Causing backward incompatibility label Jan 30, 2025

bennylp requested changes Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aligned memory allocaion #4277

aligned memory allocaion #4277

LeonidGoltsblat commented Jan 29, 2025

bennylp left a comment

bennylp Jan 30, 2025

LeonidGoltsblat Jan 30, 2025

bennylp Feb 1, 2025 •

edited

Loading

bennylp Jan 30, 2025

bennylp Jan 30, 2025

LeonidGoltsblat Jan 30, 2025

bennylp Feb 1, 2025

bennylp Jan 30, 2025

bennylp Jan 30, 2025

bennylp Jan 30, 2025

bennylp Feb 1, 2025

bennylp Jan 30, 2025

bennylp Feb 1, 2025

bennylp Jan 30, 2025

bennylp Jan 30, 2025 •

edited

Loading

bennylp commented Jan 30, 2025

LeonidGoltsblat commented Jan 30, 2025

bennylp commented Jan 30, 2025

		@@ -20,6 +20,8 @@

		#include <pj/string.h>

		#define ALIGN_PTR(PTR,ALIGNMENT) (PTR + (-(pj_ssize_t)(PTR) & (ALIGNMENT-1)))

aligned memory allocaion #4277

Are you sure you want to change the base?

aligned memory allocaion #4277

Conversation

LeonidGoltsblat commented Jan 29, 2025

bennylp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennylp Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennylp Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

bennylp commented Jan 30, 2025

LeonidGoltsblat commented Jan 30, 2025

bennylp commented Jan 30, 2025

bennylp Feb 1, 2025 •

edited

Loading

bennylp Jan 30, 2025 •

edited

Loading