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

Logging: Add support to new cbprintf packaging features #43520

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

nordic-krch
Copy link
Contributor

@nordic-krch nordic-krch commented Mar 8, 2022

Aligned logging to use static packaging whenever possible. It uses new cbprintf feature which allows to use static packaging when log message contains %s arguments. This has two main advantages:

  • it is faster - log_bechmark test shows 50% improvement for logging with single %s argument in the message.
  • logging string literal is not used in runtime which enables removing them from the binary for some backends.

PR is based on #43150 with one additional commit.

DNM because it is based on #44100.

@github-actions github-actions bot added area: API Changes to public APIs area: Documentation area: Logging area: Tests Issues related to a particular existing or missing test labels Mar 8, 2022
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch 2 times, most recently from d0f14ec to 078e023 Compare March 14, 2022 10:30
@nordic-krch nordic-krch marked this pull request as ready for review March 14, 2022 10:32
@nordic-krch nordic-krch requested a review from nashif as a code owner March 14, 2022 10:32
dcpleung
dcpleung previously approved these changes Mar 14, 2022
@github-actions github-actions bot added area: ARC ARC Architecture area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture labels Mar 22, 2022
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch from 4d2310e to dd65b13 Compare March 23, 2022 06:11
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch from dd65b13 to dec6aa3 Compare March 23, 2022 06:36
@nordic-krch nordic-krch added the DNM This PR should not be merged (Do Not Merge) label Mar 23, 2022
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch from dec6aa3 to 8192c99 Compare March 23, 2022 07:11
@MaureenHelm MaureenHelm requested a review from aasthagr March 23, 2022 14:32
dcpleung
dcpleung previously approved these changes Mar 28, 2022
teburd
teburd previously approved these changes Mar 30, 2022
@nordic-krch nordic-krch dismissed stale reviews from teburd and dcpleung via f008b8f March 30, 2022 08:56
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch from 8192c99 to f008b8f Compare March 30, 2022 08:56
@nordic-krch nordic-krch removed the DNM This PR should not be merged (Do Not Merge) label Mar 30, 2022
@nordic-krch
Copy link
Contributor Author

@teburd @dcpleung rebased, conflicts resolved automatically. Can you reapprove?

@dcpleung
Copy link
Member

I think this needs to be rebased again.

Adapt logging to always use static packaging. Runtime packaging
is used only when configuration requires that. Static packaging
significantly speeds up logging when there are string arguments.

Update log_stack test to new stack usage.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
Size of wrong string was used. It was not seen since
length was later on aligned but may lead to failures
in the future.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
@nordic-krch nordic-krch force-pushed the logging_static_packaging branch from f008b8f to 6ec78cf Compare March 30, 2022 14:39
@nordic-krch
Copy link
Contributor Author

rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARC ARC Architecture area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Documentation area: Logging area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants