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.

Sign up for GitHub

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

Jump to bottom

Aligned memory allocation #4277

Merged
merged 7 commits into from
Feb 6, 2025
Merged

Aligned memory allocation #4277

merged 7 commits into from
Feb 6, 2025

Conversation

LeonidGoltsblat
Copy link
Contributor

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 bennylp added the note:api-break Causing backward incompatibility label Jan 30, 2025
bennylp
bennylp requested changes Jan 30, 2025
View reviewed changes
Copy link
Member

@bennylp bennylp left a comment

Choose a reason for hiding this comment

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

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

pjlib/include/pj/pool_i.h Outdated Show resolved Hide resolved
pjlib/include/pj/pool_i.h Outdated Show resolved Hide resolved
pjlib/include/pj/pool_i.h Show resolved Hide resolved
pjlib/include/pj/pool_i.h Outdated Show resolved Hide resolved
pjlib/src/pj/pool_caching.c Outdated Show resolved Hide resolved
pjlib/src/pjlib-test/pool.c Outdated Show resolved Hide resolved
pjlib/src/pjlib-test/pool.c Outdated Show resolved Hide resolved
pjlib/src/pj/pool.c Outdated Show resolved Hide resolved
@bennylp
Copy link
Member

bennylp commented Jan 30, 2025

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

@LeonidGoltsblat
Copy link
Contributor Author

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
Copy link
Member

bennylp commented Jan 30, 2025

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.

bennylp and others added 4 commits February 1, 2025 08:53
@bennylp
pool test: add testing for bug in pj_pool_allocate_find with big alig…
4f66e57 
…nment, and refactor to use unit test API
@LeonidGoltsblat
misc fixes on code review
119460e
@LeonidGoltsblat
Merge pull request #1 from bennylp/my-pool
2f78760 
Pool Alt API and unit testing
@LeonidGoltsblat
Merge remote-tracking branch 'myfork/aligned-memory-allocaion' into a…
81262da 
…ligned-memory-allocaion

# Conflicts:
#	pjlib/src/pjlib-test/pool.c
@LeonidGoltsblat
Copy link
Contributor Author

please review

@bennylp bennylp changed the title aligned memory allocaion Aligned memory allocation Feb 6, 2025
sauwming
sauwming approved these changes Feb 6, 2025
View reviewed changes
Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

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

minor only

pjlib/include/pj/pool.h Outdated
@@ -331,6 +331,10 @@ struct pj_pool_t
/** The callback to be called when the pool is unable to allocate memory. */
pj_pool_callback *callback;

/** The default alignment of memory block allocated from this pool
* (must be power of 2). */
size_t alignment;
Copy link
Member

Choose a reason for hiding this comment

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

pj_size_t ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! However see also line 952

    /**
     * Internal pool.
     */
    char            pool_buf[256 * (sizeof(size_t) / 4)];

pjlib/include/pj/pool_alt.h Outdated
@@ -46,6 +46,7 @@ struct pj_pool_t
char obj_name[32];
pj_size_t used_size;
pj_pool_callback *cb;
size_t alignment;
Copy link
Member

Choose a reason for hiding this comment

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

pj_size_t

pjlib/src/pj/pool.c
PJ_ASSERT_RETURN(!alignment || PJ_IS_POWER_OF_TWO(alignment), NULL);

if (alignment < PJ_POOL_ALIGNMENT)
alignment = PJ_POOL_ALIGNMENT;
Copy link
Member

Choose a reason for hiding this comment

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

It seems this is not specified in the doc, that alignment can't be smaller than PJ_POOL_ALIGNMENT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to doc

@LeonidGoltsblat
Copy link
Contributor Author

  • pool_dbg alignment support
  • incompatible tests disabled for PJ_HAS_POOL_ALT_API

@bennylp bennylp merged commit e6196ad into pjsip:master Feb 6, 2025
39 of 42 checks passed
trengginas added a commit that referenced this pull request Feb 13, 2025
@trengginas
Squashed commit of the following:
da00a83 
commit 1e2f121
Author: sauwming <ming@teluu.com>
Date:   Thu Feb 13 10:30:58 2025 +0800

    Fixed CI Mac failure (#4304)

commit 10b4d30
Author: sauwming <ming@teluu.com>
Date:   Thu Feb 13 09:06:54 2025 +0800

    Audio and video stream refactoring (#4300)

commit 6cbf0e6
Author: Riza Sulistyo <trengginas@users.noreply.github.com>
Date:   Wed Feb 12 15:23:37 2025 +0700

    Check if ice strans is valid before using it to send (#4301)

commit fdd4041
Author: Maciej Lisowski <39798354+MaciejDromin@users.noreply.github.com>
Date:   Tue Feb 11 12:48:17 2025 +0100

    Add missing OnTimerParam import in Android Example (#4299)

commit 4ded10f
Author: sauwming <ming@teluu.com>
Date:   Fri Feb 7 13:27:00 2025 +0800

    Fixed msg_data assertion in pjsua_acc_send_request() API (#4298)

commit 65c4bc9
Author: sauwming <ming@teluu.com>
Date:   Fri Feb 7 07:18:22 2025 +0800

    Fix pjsua sample app user agent (#4296)

commit c53ace9
Author: sauwming <ming@teluu.com>
Date:   Fri Feb 7 07:18:09 2025 +0800

    Fixed Java make clean error (#4297)

commit e6196ad
Author: LeonidGoltsblat <138720759+LeonidGoltsblat@users.noreply.github.com>
Date:   Fri Feb 7 02:15:10 2025 +0300

    Aligned memory allocation (#4277)

    * aligned memory allocaion

    * Fix alt API implementations (PJ_HAS_POOL_ALT_API)

    * pool test: add testing for bug in pj_pool_allocate_find with big alignment, and refactor to use unit test API

    * misc fixes on code review

    * pool_dbg alignment support + incompatible tests disabled for PJ_HAS_POOL_ALT_API

    ---------

    Co-authored-by: bennylp <bennylp@pjsip.org>

commit 2fff775
Author: sauwming <ming@teluu.com>
Date:   Thu Feb 6 13:11:01 2025 +0800

    Add API to register custom SDP comparison callback (#4286)

commit 99b4d1e
Author: sauwming <ming@teluu.com>
Date:   Thu Feb 6 13:10:38 2025 +0800

    Fixed issue with SDP version when reoffer is rejected (#4289)

commit 0252152
Author: Benny Prijono <bennylp@pjsip.org>
Date:   Thu Feb 6 11:09:20 2025 +0700

    Use cirunner to capture and analyze GitHub action CI crash (#4288)

    * Windows runner implementation

    * Set timeout

    * Remove initial implementation of ci-runner here (it is on separate repo now)

    * Remove crash handling (-n) in main.c of unit tests

    * Install cirunner to CI workflows

    * Adding crash to timestamp test

    * Fix missing cirunner in one of the job

    * Reinstall core_pattern on Linux

    * Removed intentional crash in timestamp_test()

    * Upload program and core dump on crash

    * Add crash code in uri_test.c

    * Removed injected crash in uri_test. Disable stdout/stderr buffering for unit tests

    * Minor: remove space left out by previous clean up

commit 3fcce51
Author: sauwming <ming@teluu.com>
Date:   Wed Feb 5 11:43:27 2025 +0800

    Fixed OpenSSL log error reading cert (#4291)

commit cbfbbc4
Author: jimying <yingqw.js@gmail.com>
Date:   Wed Feb 5 11:03:53 2025 +0800

    iocp: fix crash, GetQueuedCompletionStatus() write freed WSAOVERLAPPED memory (#4136)

commit 205baf0
Author: sauwming <ming@teluu.com>
Date:   Tue Feb 4 17:04:25 2025 +0800

    Fixed warnings in sip auth client (#4287)

commit abffe0d
Author: sauwming <ming@teluu.com>
Date:   Tue Feb 4 08:38:55 2025 +0800

    Fixed CI test failure (#4284)

commit 986fc78
Author: Johannes <johannes.westhuis@gmail.com>
Date:   Mon Feb 3 08:31:28 2025 +0100

    Share an auth session between multiple dialogs/regc (#4262)

commit 46111c4
Author: Nanang Izzuddin <nanang@teluu.com>
Date:   Mon Feb 3 11:36:54 2025 +0700

    Best effort avoid crash when media transport adapter not using group lock (#4281)

commit f986ad8
Author: Benny Prijono <bennylp@pjsip.org>
Date:   Fri Jan 31 09:45:19 2025 +0700

    Add link to coding style documentation (#4280)

commit 727ee32
Author: Nanang Izzuddin <nanang@teluu.com>
Date:   Fri Jan 31 09:08:02 2025 +0700

    Fix build error when PJ_LOG_MAX_LEVEL is zero (#4279)

    The `pj_log_get_log_func()` is not defined when PJ_LOG_MAX_LEVEL is set to zero.

    Thanks to Giorgio Alfarano for the report.

commit dae52f6
Author: Perry Ismangil <perry@teluu.com>
Date:   Thu Jan 30 08:43:54 2025 +0000

    Fixing typo (#4274)

    Acoustic

commit 1a4cd67
Author: sauwming <ming@teluu.com>
Date:   Thu Jan 30 15:21:49 2025 +0800

    Modify iOS sample apps dev team ID (#4278)

commit dfcfa13
Author: Tarteszeus <37761609+Tarteszeus@users.noreply.github.com>
Date:   Thu Jan 30 02:42:36 2025 +0100

    Add queried names to server address record, and add the address record in parameter for on_verify_cb callback (#4256)

commit f9e56d8
Author: Jan Tojnar <jtojnar@gmail.com>
Date:   Wed Jan 29 07:42:18 2025 +0100

    Fix duplicate function name in 100rel docs (#4275)

commit 960597e
Author: Nanang Izzuddin <nanang@teluu.com>
Date:   Wed Jan 29 13:36:34 2025 +0700

    Various works on SWIG Java (#4273)

    * Various works on SWIG Java

    1. Fix type mapping (SWIGTYPE_*):
       a. Map C "void*" & "void**" to Java long (was SWIGTYPE_p_void & SWIGTYPE_p_p_void which are not really usable), this should fix #4242.
       b. Map pjmedia_aud_dev_index to int.
       c. Map unsigned char[20] for SslCertInfo.serialNo to Java "short array"

       This also updates pjsua.i, e.g: tab->space, reorder things.

    2. Update swig_java_pjsua2.vcxproj:
       a. Rename config "Debug" & "Release" to "Debug-Dynamic" & "Release-Dynamic" in , as the project actually builds dynamic libs. Also fix the property sheet dependencies from *-static to *-dynamic.
       b. Update other settings, e.g: built tool version from 140 to 143.

    3. Update symbols.lst: added missing new types, tab->space, reorder alphabetically.

    * Update ci-win.yml
    * Add sample code for passing user data using utilTimerSchedule()
    * Add sample for cancelling timer

commit c36ed2c
Author: Benny Prijono <bennylp@pjsip.org>
Date:   Tue Jan 28 16:58:55 2025 +0700

    Minor modifications to Android build and samples to match new documentation (#4271)

    * To streamline the command, also clean swig and pjsua jni output directories when make distclean and realclean is called

    * Kotlin sample: add account, modify video size and bandwidth, and audio codec priorities to use AMR-WB

    * Android CLI app: fix armeabi hardcoded arch and also copy stdc++.so

commit bab33d6
Author: Noel Morgan <noel@vwci.com>
Date:   Tue Jan 28 01:51:50 2025 -0600

    Added support for updated RFC7866 content-type sub type with XML extension (#4270)

commit a89917e
Author: sauwming <ming@teluu.com>
Date:   Fri Jan 24 14:49:59 2025 +0800

    OpenSSL: Set ciphersuites only if not using BoringSSL (#4269)

commit 377a80c
Author: Nanang Izzuddin <nanang@teluu.com>
Date:   Fri Jan 24 13:48:31 2025 +0700

    Fix various compile errors & warnings in MSVC2005 (#4268)

commit de3f2e1
Author: sauwming <ming@teluu.com>
Date:   Fri Jan 24 10:20:28 2025 +0800

    Set CI vars in GH workflow file (#4263)

commit cdb1294
Author: sauwming <ming@teluu.com>
Date:   Thu Jan 23 11:01:27 2025 +0800

    Various fixes for Apple SSL backend (#4257)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennylp bennylp bennylp approved these changes

@sauwming sauwming sauwming approved these changes

@nanangizz nanangizz nanangizz approved these changes

Assignees
No one assigned
Labels
component: pjlib note:api-break Causing backward incompatibility type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LeonidGoltsblat @bennylp @sauwming @nanangizz