Prefer dynamic allocations over large STATIC arrays #2146

vikramdattu · 2025-06-05T14:44:20Z

Issue #, if available:

N/A

What was changed?

Use dynamically allocated WSS URL
Use dynamic allocations for singaling payload
Use required only allocations in LwsApiCalls instead large static arrays

Why was it changed?

Dynamic SignalingPayload: We end up allocating MAX_SIGNALING_MESSAGE_LEN per signaling message received. For memory constrained devices, this is a huge deal. and would reduce(~couple of 100KBs of) peak memory usage.
Having exact allocations while doing API calls saves us needing unreasonably high memory needs on stack

How was it changed?

Provided an option PREFER_DYNAMIC_ALLOCS cmake option. Enabling this, turns dynamic allocations ON instead of large fixed arrays

vikramdattu · 2025-06-05T14:46:47Z

@sirknightj

this is comprehensive change to use dynamic allocations instead of huge static arrays.
Support dynamically allocated signaling payload #2140 will be closed in favour of this

I am having the thought of simply keeping dynamic allocations and remove the old code if that makes sense.
Saves us from maintaining the switch.

vikramdattu · 2025-06-17T14:48:55Z

@sirknightj @unicornss would love to get your review on this particular PR.

sirknightj

Good memory optimizations enhancements that reduce the allocated memory per URL from:

#define MAX_URI_CHAR_LEN 10000
#define MAX_JSON_PARAMETER_STRING_LEN (10 * 1024)

about 20k

Down to whatever is required based on the calculated length

src/source/Signaling/LwsApiCalls.c

sirknightj · 2025-06-17T17:53:11Z

src/source/Signaling/LwsApiCalls.c

-
+#ifdef DYNAMIC_SIGNALING_PAYLOAD
+        SAFE_MEMFREE(pSignalingMessageWrapper->receivedSignalingMessage.signalingMessage.payload);
+#endif


In the sample application, would you be able to demo how this one works for others with the ifdef, maybe add something in the readme about the new option.

Hi @sirknightj I have now made the relevant changes in Common.c as well. Also, made changes in Tests to account for dynamic payload.
You may pass -DPREFER_DYNAMIC_ALLOCS=ON to cmake and try out the sample application as well as run the the SignalingApiTest.

Thank you, I will give it a try

I ran the sample and it works fine.

The test is failing for me oddly enough:

./tst/webrtc_client_test --gtest_filter="SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess" Note: Google Test filter = SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from SignalingApiFunctionalityTest [ RUN ] SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess Address of receivedMessage: 0x16b3c5f18 Address of payload member: 0x16b3c6128 Allocation address: 0x600000eac0e0 Allocated payload points to: 0x600000eac0e0 About to free pointer: 0x600000eac0e0 webrtc_client_test(98158,0x20bebcc80) malloc: *** error for object 0x600000eac0d8: pointer being freed was not allocated webrtc_client_test(98158,0x20bebcc80) malloc: *** set a breakpoint in malloc_error_break to debug [1] 98158 abort ./tst/webrtc_client_test

It seems like 8 bytes went missing.

(lldb) frame select 6 frame #6: 0x00000001008838b4 libkvsWebrtcClient.dylib`defaultMemFree(ptr=0x0000600000f20048) at Allocators.c:36:5 (lldb) p ptr (void *) 0x0000600000f20048 (lldb) frame select 8 frame #8: 0x000000010014aa90 webrtc_client_test`com::amazonaws::kinesis::video::webrtcclient::SignalingApiFunctionalityTest_SignalingMessageParsing_ParseOfferSuccess_Test::TestBody(this=0x000000012b809400) at SignalingApiFunctionalityTest.cpp:3657:5 3654 EXPECT_EQ(SIGNALING_MESSAGE_TYPE_OFFER, receivedMessage.signalingMessage.messageType); 3655 #ifdef DYNAMIC_SIGNALING_PAYLOAD 3656 printf("About to free pointer: %p\n", receivedMessage.signalingMessage.payload); -> 3657 SAFE_MEMFREE(receivedMessage.signalingMessage.payload); 3658 #endif 3659 } 3660 (lldb) p receivedMessage.signalingMessage.payload error: <user expression 5>:1:17: no member named 'signalingMessage' in '(unnamed struct)' 1 | receivedMessage.signalingMessage.payload | ~~~~~~~~~~~~~~~ ^ (lldb) x/4gx &receivedMessage.signalingMessage.payload 0x16fdfdea8: 0x0000600000f20050 0x0000000000000000 0x16fdfdeb8: 0x0000000000000000 0x0000000000000000 (lldb) memory read --size 8 --format x --count 4 receivedMessage.signalingMessage.payload 0x600000f20050: 0x0000006f6c6c6548 0x0000000000000000 0x600000f20060: 0x0000000000000000 0x0000000000000000 (lldb)

0x6f6c6c6548 --> (base64) b2xsZUg= --> (ascii) olleH

I was able to get around it by replacing the SAFE_MEMFREE in that test with a direct free(). MEMFREE seems to also run into the segfault. Though, that doesn't seem ideal.

Would you be able to add these to the CI? To run the unit tests and also the sample in the new mode.

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fc822e155..86bd87f41f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -278,6 +278,8 @@ jobs: cmake-args: "-DUSE_OPENSSL=ON -DUSE_MBEDTLS=OFF" - name: lws cmake-args: "-DUSE_OPENSSL=OFF -DUSE_MBEDTLS=ON" + - name: dynamic-allocs + cmake-args: "-DPREFER_DYNAMIC_ALLOCS=ON" fail-fast: false runs-on: ubuntu-latest diff --git a/.github/workflows/samples.yaml b/.github/workflows/samples.yaml index c1815230bb..c25551681a 100644 --- a/.github/workflows/samples.yaml +++ b/.github/workflows/samples.yaml @@ -108,7 +108,7 @@ jobs: - name: Build repository run: | mkdir build && cd build - cmake .. -DUSE_OPENSSL=OFF -DUSE_MBEDTLS=ON + cmake .. -DUSE_OPENSSL=OFF -DUSE_MBEDTLS=ON -DPREFER_DYNAMIC_ALLOCS=ON make -j - name: Run tests

Thanks, the crash looks strange indeed. I will give some more iterations of tests to reproduce the issue and find a fix.

Sure, will add a new test in CI with PREFER_DYNAMIC_ALLOCS=ON

Did a bit more digging, though I have not found a root cause yet.

Tested on M1 Pro Macbook -- built with: cmake .. -DUSE_OPENSSL=OFF -DUSE_MBEDTLS=ON -DCMAKE_BUILD_TYPE=Debug -DBUILD_TEST=ON -DENABLE_AWS_SDK_IN_TESTS=OFF -DPREFER_DYNAMIC_ALLOCS=ON

Running with address sanitizer:

./tst/webrtc_client_test --gtest_filter="SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess" webrtc_client_test(84998,0x20bebcc80) malloc: nano zone abandoned due to inability to reserve vm space. Note: Google Test filter = SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from SignalingApiFunctionalityTest [ RUN ] SignalingApiFunctionalityTest.SignalingMessageParsing_ParseOfferSuccess ================================================================= ==84998==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x6020000034c8 in thread T0 #0 0x1061b8d40 in free+0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x54d40) #1 0x105bc5594 in defaultMemFree Allocators.c:36 #2 0x105bc9f58 in instrumentedAllocatorsMemFree InstrumentedAllocators.c:187 #3 0x104f281f8 in com::amazonaws::kinesis::video::webrtcclient::SignalingApiFunctionalityTest_SignalingMessageParsing_ParseOfferSuccess_Test::TestBody() SignalingApiFunctionalityTest.cpp:3650 #4 0x10505281c in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x90 (webrtc_client_test:arm64+0x10051681c) #5 0x10501c9c0 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x5c (webrtc_client_test:arm64+0x1004e09c0) #6 0x10501c910 in testing::Test::Run()+0xbc (webrtc_client_test:arm64+0x1004e0910) #7 0x10501d860 in testing::TestInfo::Run()+0x12c (webrtc_client_test:arm64+0x1004e1860) #8 0x10501e9c8 in testing::TestSuite::Run()+0x144 (webrtc_client_test:arm64+0x1004e29c8) #9 0x10502c478 in testing::internal::UnitTestImpl::RunAllTests()+0x420 (webrtc_client_test:arm64+0x1004f0478) #10 0x1050593a0 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x90 (webrtc_client_test:arm64+0x10051d3a0) #11 0x10502be18 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x5c (webrtc_client_test:arm64+0x1004efe18) #12 0x10502bd04 in testing::UnitTest::Run()+0xd4 (webrtc_client_test:arm64+0x1004efd04) #13 0x104ffb350 in RUN_ALL_TESTS() gtest.h:2293 #14 0x104ffb1c8 in main main.cpp:88 #15 0x19cc4ab48 (<unknown module>) 0x6020000034c8 is located 8 bytes before 7-byte region [0x6020000034d0,0x6020000034d7) allocated by thread T0 here: #0 0x1061b8fd0 in calloc+0x9c (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x54fd0) #1 0x10565ab70 in defaultMemCalloc+0x1c (libkvsCommonLws.1.5.4.dylib:arm64+0xeb70) #2 0x1056d9370 in parseSignalingMessage LwsApiCalls.c:2207 #3 0x104f276b8 in com::amazonaws::kinesis::video::webrtcclient::SignalingApiFunctionalityTest_SignalingMessageParsing_ParseOfferSuccess_Test::TestBody() SignalingApiFunctionalityTest.cpp:3642 #4 0x10505281c in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x90 (webrtc_client_test:arm64+0x10051681c) #5 0x10501c9c0 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x5c (webrtc_client_test:arm64+0x1004e09c0) #6 0x10501c910 in testing::Test::Run()+0xbc (webrtc_client_test:arm64+0x1004e0910) #7 0x10501d860 in testing::TestInfo::Run()+0x12c (webrtc_client_test:arm64+0x1004e1860) #8 0x10501e9c8 in testing::TestSuite::Run()+0x144 (webrtc_client_test:arm64+0x1004e29c8) #9 0x10502c478 in testing::internal::UnitTestImpl::RunAllTests()+0x420 (webrtc_client_test:arm64+0x1004f0478) #10 0x1050593a0 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x90 (webrtc_client_test:arm64+0x10051d3a0) #11 0x10502be18 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x5c (webrtc_client_test:arm64+0x1004efe18) #12 0x10502bd04 in testing::UnitTest::Run()+0xd4 (webrtc_client_test:arm64+0x1004efd04) #13 0x104ffb350 in RUN_ALL_TESTS() gtest.h:2293 #14 0x104ffb1c8 in main main.cpp:88 #15 0x19cc4ab48 (<unknown module>) SUMMARY: AddressSanitizer: bad-free (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x54d40) in free+0x98 ==84998==ABORTING [1] 84998 abort ./tst/webrtc_client_test

Replacing the SAFE_MEMFREE with:

MEMFREE -- ❌

globalMemFree -- ❌

defaultMemFree -- ✅

free -- ✅

Strange that it only shows up in the tests and not in the sample..

I added this immediately above the test (SignalingApiFunctionalityTest:3631) and it seems to fix the issue:

VOID safeMemFree(VOID* ptr) { defaultMemFree(ptr); } memFree globalMemFree = safeMemFree;

So it would be logical to conclude that the issue might be related to the type definitions or maybe an <include> ordering issue?

In PIC, PVOID and VOID* are declared the same -
https://github.com/awslabs/amazon-kinesis-video-streams-pic/blob/c98c2a256a7bb3dfc4db41ae26d45e28ac7eec56/src/common/include/com/amazonaws/kinesis/video/common/CommonDefs.h#L183

@sirknightj Eureka!!
I found out the reason! Went ahead debugging with lldb and found out:

The tests use instrumentedAllocator versions of allocators whereas the SDK is using global Allocators.
This free here is decrementing the pointer before free: https://github.com/awslabs/amazon-kinesis-video-streams-pic/blob/c98c2a256a7bb3dfc4db41ae26d45e28ac7eec56/src/utils/src/InstrumentedAllocators.c#L181

Interesting finding!

I do see that in the test setup, it call setInstrumentedAllocators(), so the MEMCALLOC should have been set to point to the allocated one to match..

amazon-kinesis-video-streams-webrtc-sdk-c/tst/WebRTCClientTestFixture.cpp

Line 57 in cf817bc

SET_INSTRUMENTED_ALLOCATORS();

And initKvsWebrtc() also calls it:

amazon-kinesis-video-streams-webrtc-sdk-c/tst/WebRTCClientTestFixture.cpp

Line 68 in cf817bc

if (STATUS_SUCCESS != initKvsWebRtc()) {

amazon-kinesis-video-streams-webrtc-sdk-c/src/source/PeerConnection/PeerConnection.c

Line 1773 in cf817bc

SET_INSTRUMENTED_ALLOCATORS();

MushMal · 2025-06-17T19:02:46Z

I believe this is a good change. Previously, the codebase was optimized to avoid dynamic allocations due to the cost impact on many small footprint platforms in favor of the predictable memory footprint. Most platforms do provide a dynamic memory allocators and if a particular platform still has eprom type of memory only, the allocators for the SDK can be overridden to utilize the sub-allocator the SDK provides (AIV heap implementation). There are many samples of this in the tests that validate the memory leakage.

src/source/Signaling/LwsApiCalls.c

- Use dynamically allocated WSS URL - Use dynamic allocations for singaling payload - Use required only allocations in LwsApiCalls instead large static arrays

vikramdattu · 2025-06-18T11:32:25Z

src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h

+ */
+#if PREFER_DYNAMIC_ALLOCS
+#define DYNAMIC_SIGNALING_PAYLOAD 1
+#define USE_DYNAMIC_URL 1


TODO: Remove this! Not used anymore.

vikramdattu · 2025-06-18T12:06:41Z

src/source/Signaling/LwsApiCalls.c

-    CHAR paramsJson[MAX_JSON_PARAMETER_STRING_LEN];
+    UINT32 paramsJsonLen = 0;
+    PCHAR url = NULL;
+    PCHAR paramsJson = NULL;
    CHAR tagsJson[2 * MAX_JSON_PARAMETER_STRING_LEN];


TODO: Fix this as well. 2 * MAX_JSON_PARAMETER_STRING_LEN is 20K

vikramdattu mentioned this pull request Jun 5, 2025

Support dynamically allocated signaling payload #2140

Draft

vikramdattu force-pushed the optimize/option_to_prefer_dynamic_allocs_over_static branch from 49be34e to 4f3e2a5 Compare June 10, 2025 05:28

unicornss requested review from stefankiesz, sirknightj and unicornss June 17, 2025 15:34

sirknightj reviewed Jun 17, 2025

View reviewed changes

src/source/Signaling/LwsApiCalls.c Show resolved Hide resolved

vikramdattu force-pushed the optimize/option_to_prefer_dynamic_allocs_over_static branch from 4f3e2a5 to 1cf9fff Compare June 18, 2025 11:31

Prefer dynamic allocations over large STATIC arrays

3d7ad7b

- Use dynamically allocated WSS URL - Use dynamic allocations for singaling payload - Use required only allocations in LwsApiCalls instead large static arrays

vikramdattu force-pushed the optimize/option_to_prefer_dynamic_allocs_over_static branch from 1cf9fff to 3d7ad7b Compare June 18, 2025 12:13

vikramdattu commented Jun 18, 2025

View reviewed changes

vikramdattu requested a review from sirknightj June 18, 2025 15:29

Prefer dynamic allocations over large STATIC arrays #2146

Are you sure you want to change the base?

Prefer dynamic allocations over large STATIC arrays #2146

Uh oh!

Conversation

vikramdattu commented Jun 5, 2025

Uh oh!

vikramdattu commented Jun 5, 2025

Uh oh!

vikramdattu commented Jun 17, 2025

Uh oh!

sirknightj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirknightj Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MushMal commented Jun 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sirknightj Jun 19, 2025 •

edited

Loading