trace-load: improve trace load lookup #688

alexmohr · 2024-10-16T15:17:49Z

Store trace load in context of doing a search on each message.

The increased load was detected by our performance team.
Please note that our internal version is not building on master but a previous release, thus I had to adjust the patch a little.
I wanted to upstream this change asap although it needs to go through some final verification on our end because it is a very important amendment to the existing implementation and should be a huge from a performance viewpoint.

@minminlittleshrimp Please review this and also give it a test in your setup.

I'll be on vacation starting at the end of next week, so response times will be a lot longer than usual and I won't be able to make changes before my return at the start of December.
Furthermore I will be switching Teams internally at the start of next year, so this might be one of my last contributions for some time.
It has been really fun working together with you :)

solves #685

The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0

^{Alexander Mohr, [email protected], Mercedes-Benz Tech Innovation GmbH, imprint}

alexmohr · 2024-10-16T16:23:58Z

quick side note on these changes:

strncpy(daemon.preconfigured_trace_load_settings[0].apid, "APP0", DLT_ID_SIZE);

while not strictly required with the compiler used in the pipeline it crashed locally with the following gcc version

gcc (Ubuntu 10.5.0-1ubuntu1~20.04) 10.5.0
Copyright (C) 2020 Free Software Foundation, Inc.

Issue-ID: APRICOT-631667 Signed-off-by: Alexander Mohr <[email protected]>

Bichdao021195 · 2024-10-30T10:32:17Z

Store trace load in context of doing a search on each message.

The increased load was detected by our performance team. Please note that our internal version is not building on master but a previous release, thus I had to adjust the patch a little. I wanted to upstream this change asap although it needs to go through some final verification on our end because it is a very important amendment to the existing implementation and should be a huge from a performance viewpoint.

@minminlittleshrimp Please review this and also give it a test in your setup.

I'll be on vacation starting at the end of next week, so response times will be a lot longer than usual and I won't be able to make changes before my return at the start of December. Furthermore I will be switching Teams internally at the start of next year, so this might be one of my last contributions for some time. It has been really fun working together with you :)

solves #685

Hi @alexmohr
Thanks for your pull request. Can you provide the example steps how to verify the trace load improvement with your code change if it's possible?

Bichdao021195 · 2024-10-31T10:41:53Z

Store trace load in context of doing a search on each message.

The increased load was detected by our performance team. Please note that our internal version is not building on master but a previous release, thus I had to adjust the patch a little. I wanted to upstream this change asap although it needs to go through some final verification on our end because it is a very important amendment to the existing implementation and should be a huge from a performance viewpoint.

@minminlittleshrimp Please review this and also give it a test in your setup.

I'll be on vacation starting at the end of next week, so response times will be a lot longer than usual and I won't be able to make changes before my return at the start of December. Furthermore I will be switching Teams internally at the start of next year, so this might be one of my last contributions for some time. It has been really fun working together with you :)

solves #685

Hi @alexmohr
I noticed that the trace load setting address is stored in the DLT context, allowing searches on each message using the dlt_daemon_context_find API instead of directly calling dlt_find_runtime_trace_load_setting. Could you please explain why this approach helps reduce CPU load? Because we still need to search the context on each message for using trace load setting .

alexmohr · 2024-10-31T16:27:25Z

Testing can be done the same as the original Pr. i can't provide better tests at the moment because I'm not in office until December.

Using this approach is faster because this api is using qsearch instead of a linear search. Most improvements are on clients side though where total number of search operations are reduced drastically by storing the trace load limits in the context.

Bichdao021195 · 2024-11-05T06:15:45Z

src/lib/dlt_user.c

-                    dlt_find_runtime_trace_load_settings(
+                dlt_ll_ts_type* ll_ts = &dlt_user.dlt_ll_ts[log->handle->log_level_pos];
+                if (ll_ts->trace_load_settings == NULL) {
+                    ll_ts->trace_load_settings = dlt_find_runtime_trace_load_settings(


The trace load settings should be check null before accessing

Can you elaborate on this? Null check for tl settings is above and this if is only executed when it is null

Do you mean checking ll_ts for null? I think that check would be superfluous

After carefully checking, it is superfluous to check ll_ts->trace_load_settings null. Because this pointer already check null and print log inside func "dlt_check_trace_load"

Bichdao021195 · 2024-11-05T06:48:33Z

src/daemon/dlt_daemon_common.h

@@ -162,7 +162,10 @@ typedef struct
    int user_handle;    /**< connection handle for connection to user application */
    char *context_description; /**< context description */
    int8_t storage_log_level; /**< log level set for offline logstorage */
-    bool predefined; /**< set to true if this context is predefined by runtime configuration file */
+    bool predefined;


"Could you explain the purpose of removing the comment of the bool predefined variable? If it was removed intentionally, kindly please consider reverting this line if it is not necessary."

Was not used as far I could tell, but maybe missed something?

Bichdao021195 · 2024-11-05T06:48:43Z

src/daemon/dlt-daemon.c

@@ -1636,6 +1636,13 @@ int main(int argc, char *argv[])
    dlt_gateway_deinit(&daemon_local.pGateway, daemon_local.flags.vflag);

    dlt_daemon_free(&daemon, daemon_local.flags.vflag);
+#ifdef DLT_TRACE_LOAD_CTRL_ENABLE


Those lines of code should be warped in 1 function like "dlt_traceload_free"

Will do, once I'm back in office in December :)

Bichdao021195 · 2024-11-05T06:51:02Z

src/lib/dlt_user.c

@@ -1308,6 +1306,15 @@ DltReturnValue dlt_register_app(const char *apid, const char *description)

 #ifdef DLT_TRACE_LOAD_CTRL_ENABLE
    strncpy(trace_load_settings[0].apid, dlt_user.appID, DLT_ID_SIZE);
+    if (!trace_load_context.contextID[0])


It is make sense to register special context inside dlt_register_app. Because dlt_register_app api serve for register app purpose.

It is required, as logging without calling the register api first is technically possible and is happening within our project and this change is implemented in a way that is fully compliant with existing code

alexmohr · 2024-11-22T18:00:24Z

src/lib/dlt_user.c

@@ -1308,6 +1306,15 @@ DltReturnValue dlt_register_app(const char *apid, const char *description)

 #ifdef DLT_TRACE_LOAD_CTRL_ENABLE
    strncpy(trace_load_settings[0].apid, dlt_user.appID, DLT_ID_SIZE);


Suggested change

strncpy(trace_load_settings[0].apid, dlt_user.appID, DLT_ID_SIZE);

pthread_rwlock_wrlock(&trace_load_rw_lock);

strncpy(trace_load_settings[0].apid, dlt_user.appID, DLT_ID_SIZE);

pthread_rwlock_unlock(&trace_load_rw_lock);

Writing settings needs protection to prevent seg faults

alexmohr · 2024-12-02T16:26:03Z

this is a possible deadlock that still needs to be fixed. I'm working internally on a fix and will provide this along with the other changes as soon as it's working internally.

alexmohr · 2024-12-06T17:57:05Z

fixed the deadlock, fixed findings.
@Bichdao021195 can you give it another round of review?

Bichdao021195 · 2024-12-09T08:51:10Z

fixed the deadlock, fixed findings. @Bichdao021195 can you give it another round of review?

Thanks @alexmohr for your contribution on DLT.
If possible, could you also guide me on how to set up and monitor the trace-load improvements in your changes? I’d like to observe how the batch improves the performance of the trace-load feature. Which indicators should we monitor?
Additionally, are there any steps to reproduce the deadlock issue?

Signed-off-by: Alexander Mohr <[email protected]>

alexmohr · 2024-12-16T17:42:42Z

@Bichdao021195 just pushed some more changes for scenarios we had deadlocks in the fleet.

Which indicators should we monitor?

CPU time spend in libdlt especially in dlt_find_runtime_trace_load_settings was a lot prior to this change, as an expensive search was done for every message. This change drastically reduces the load by caching the values.
It shows best when a lot of messages are logged and filtering by context id is done.

Additionally, are there any steps to reproduce the deadlock issue?

Main issue was in dlt exit, where an internal application got stuck due to the scenario in the graph I provided.
It's possible to reproduce this with out of the box tools like dlt-adaptor-stdin as well, by running them in a loop and waiting for it not to exit.

alexmohr force-pushed the improve-trace-load branch from 8297782 to d4b2bf7 Compare October 16, 2024 16:19

alexmohr marked this pull request as draft October 16, 2024 17:52

alexmohr force-pushed the improve-trace-load branch from d4b2bf7 to 15ed64f Compare October 16, 2024 19:26

tests: fix unit tests on gcc15

96966fb

Issue-ID: APRICOT-631667 Signed-off-by: Alexander Mohr <[email protected]>

alexmohr force-pushed the improve-trace-load branch from 15ed64f to 182fbce Compare October 20, 2024 11:37

alexmohr marked this pull request as ready for review October 20, 2024 11:44

alexmohr force-pushed the improve-trace-load branch 6 times, most recently from 70ce605 to fb8c315 Compare October 23, 2024 06:43

Bichdao021195 reviewed Nov 5, 2024

View reviewed changes

alexmohr commented Nov 22, 2024

View reviewed changes

alexmohr force-pushed the improve-trace-load branch from fb8c315 to babfa5c Compare December 6, 2024 17:54

trace-load: improve lookup speeds and reduce cpu load caused

013c3aa

Signed-off-by: Alexander Mohr <[email protected]>

alexmohr force-pushed the improve-trace-load branch from babfa5c to 013c3aa Compare December 16, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trace-load: improve trace load lookup #688

trace-load: improve trace load lookup #688

alexmohr commented Oct 16, 2024 •

edited

Loading

alexmohr commented Oct 16, 2024 •

edited

Loading

Bichdao021195 commented Oct 30, 2024

Bichdao021195 commented Oct 31, 2024

alexmohr commented Oct 31, 2024 •

edited

Loading

Bichdao021195 Nov 5, 2024 •

edited

Loading

alexmohr Nov 15, 2024

alexmohr Nov 15, 2024

Bichdao021195 Nov 15, 2024

Bichdao021195 Nov 5, 2024

alexmohr Nov 15, 2024

Bichdao021195 Nov 5, 2024

alexmohr Nov 15, 2024

Bichdao021195 Nov 5, 2024

alexmohr Nov 15, 2024

alexmohr Nov 22, 2024

alexmohr commented Dec 2, 2024

alexmohr commented Dec 6, 2024

Bichdao021195 commented Dec 9, 2024

alexmohr commented Dec 16, 2024

		@@ -1308,6 +1306,15 @@ DltReturnValue dlt_register_app(const char apid, const char description)

		#ifdef DLT_TRACE_LOAD_CTRL_ENABLE
		strncpy(trace_load_settings[0].apid, dlt_user.appID, DLT_ID_SIZE);

trace-load: improve trace load lookup #688

Are you sure you want to change the base?

trace-load: improve trace load lookup #688

Conversation

alexmohr commented Oct 16, 2024 • edited Loading

alexmohr commented Oct 16, 2024 • edited Loading

Bichdao021195 commented Oct 30, 2024

Bichdao021195 commented Oct 31, 2024

alexmohr commented Oct 31, 2024 • edited Loading

Bichdao021195 Nov 5, 2024 • 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

alexmohr commented Dec 2, 2024

alexmohr commented Dec 6, 2024

Bichdao021195 commented Dec 9, 2024

alexmohr commented Dec 16, 2024

alexmohr commented Oct 16, 2024 •

edited

Loading

alexmohr commented Oct 16, 2024 •

edited

Loading

alexmohr commented Oct 31, 2024 •

edited

Loading

Bichdao021195 Nov 5, 2024 •

edited

Loading