fix: android: do not initialize tracing if it is disabled #38956
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
f6715f3 to
c525049
Compare
|
PR #38956: Size comparison from 162df09 to c525049 Full report (3 builds for cc32xx, stm32)
|
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
c525049 to
0294d9d
Compare
|
PR #38956: Size comparison from 162df09 to 0294d9d Full report (44 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
2 times, most recently
from
6b3dc3e to
06cfe7c
Compare
|
PR #38956: Size comparison from 162df09 to 06cfe7c Full report (5 builds for cc32xx, stm32, tizen)
|
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
06cfe7c to
16032b3
Compare
|
PR #38956: Size comparison from 0238fed to 16032b3 Full report (3 builds for cc32xx, stm32)
|
andy31415
reviewed
May 14, 2025
andy31415
reviewed
May 14, 2025
|
@j0tunn until now android is always built with tracing enabled. Do you have specific needs to compile it without tracing? The question that needs to be answered in the PR summary is also why something is needed. Design currently seems to be "this platform always wants tracing". |
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
16032b3 to
5ccf921
Compare
andy31415
requested changes
May 14, 2025
There was a problem hiding this comment.
Changes requested:
- update summary to give detail about testing (since it is manual, provide full instructions for the issue to be reproducible)
- describe in the summary on why we want to support android without tracing? Is this a size difference or performance increase or a customer requested thing (do we have a reported issue for this with explanation) or something else
- fix build/CI
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
2 times, most recently
from
87c2306 to
33a7337
Compare
|
PR #38956: Size comparison from 4cb3032 to 33a7337 Full report (3 builds for cc32xx, stm32)
|
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
33a7337 to
2b22353
Compare
|
PR #38956: Size comparison from a8b1926 to 2b22353 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
May 14, 2025
src/platform/android/BUILD.gn
Outdated
| "${chip_root}/src/tracing/perfetto:event_storage", | ||
| "${chip_root}/src/tracing/perfetto:simple_initialization", | ||
| ] | ||
| deps = [ "${chip_root}/src/tracing:tracing_buildconfig" ] |
There was a problem hiding this comment.
these deps do not seem used if the source files are not compiled?
Why are we not keeping tracing source_set as is, but add a dependency on it based on matter_enable_tracing_support? And the AndroidChipPlatform-JNI is the one that uses build_config.
So we should have: static_library('android') changed:
- it requires the buildconfig dependency
- it requires to make the tracing dependency optional. Also is it needed to be excluded? Would link-time optimization not remove the tracing.h/cpp calls if they are not referenced?
andy31415
requested changes
May 14, 2025
There was a problem hiding this comment.
Request changes: build.gn should be updated:
- add dependency where you need it (not in a empty source set)
- conditional seems to be needed on the dependency addition not in wiping out the entire tracing bit
I believe the CI will guide you to do these changes as well (I see build failing without it).
Since CI is red: did you actually test your changes locally? Summary claims you did however I do not understand how that test passed when CI is red.
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
2b22353 to
7d311f0
Compare
|
PR #38956: Size comparison from c745bea to 7d311f0 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
I've tested it in on our custom build which is generated from gn files. |
andy31415
reviewed
May 15, 2025
j0tunn
force-pushed
the
fix/android.disable.tracing
branch
from
7d311f0 to
d84ae7e
Compare
|
PR #38956: Size comparison from 2ed64bc to d84ae7e Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
May 16, 2025
| } | ||
|
|
||
| public_deps = [ | ||
| "${chip_root}/src/platform:platform_base", |
There was a problem hiding this comment.
Is there a reason we need platform-base?
Can these be "deps" instead of "public_deps" (given that changes occur in cpp only, so we do not need to expose these beyond that in headers)?
There was a problem hiding this comment.
platform_base was here before my changes. I've just add tracing_buildconfig here.
andy31415
approved these changes
May 16, 2025
|
@andy31415 Can you suggest somebody who can review this PR. Looks like CI still waiting for second approve |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For now android build always include perfetto tracing. We shouldn't build it if tracing is disabled via build argument
matter_enable_tracing_support.Tracing increases the target binary size and build time. Thats a "pay-for-use" thing: do not compile tracing code into target binary if tracing is disabled.
Testing
Manual build. Steps:
matter_enable_tracing_support = false