Remove local strings from timing by simonrozsival · Pull Request #12513 · dotnet/android

simonrozsival · 2026-08-25T11:46:22Z

Summary

Remove the shared fast-timing implementation's local-string dependencies without introducing limits based on their former inline-storage thresholds.

Changes

keep fixed arrays only for genuinely bounded Android properties, logcat messages, and integer formatting
make assembly timing metadata formatting return the negative required capacity and retry with exact-size malloc() storage
keep typical metadata on the stack and free oversized returned storage only when it differs from the stack buffer
use the pointer-returning join_paths() helper for timing output
reject a missing or empty TMPDIR and close the output file on setup failure
remove timing and assembly-store strings.hh dependencies
update the affected CoreCLR APK size references from Azure DevOps build 1566719

Validation

Local native builds intentionally skipped; relying on CI validation
APK size references refreshed from Azure DevOps build 1566719

Copilot

Pull request overview

This PR refactors the shared native fast-timing implementation to stop using dynamic_local_string for formatting and option parsing, moving toward fixed-buffer (char[]) handling to reduce reliance on the local-string hierarchy and eventually C++ runtime dependencies in shared native code.

Changes:

Switch timing option retrieval/parsing (debug.mono.timing) to fixed buffers and in-place C-string tokenization.
Replace timing log/event message construction with fixed char arrays and snprintf, returning event descriptions as string literals.
Add AndroidSystem::monodroid_get_system_property overloads that write directly into caller-provided char[] buffers (MonoVM and CLR variants).

Show a summary per file

File	Description
src/native/mono/runtime-base/android-system.hh	Adds `char[]` overload for system property retrieval on the MonoVM side.
src/native/common/runtime-base/timing-internal.cc	Migrates timing option parsing, log message formatting, and output path building to fixed buffers.
src/native/common/include/runtime-base/timing-internal.hh	Updates timing message formatting helpers to use fixed buffers and `snprintf`.
src/native/clr/include/runtime-base/android-system.hh	Adds `char[]` overload for system property retrieval on the CLR side.

Review details

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The fixed timing buffers reduce libmonodroid.so from 1,184,800 to 1,094,848 bytes. Refresh all four affected CoreCLR package baselines using build 1566719 output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Assembly store timing metadata no longer uses the local-string types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Allocate timing metadata to its exact size instead of treating the former local-string stack threshold as a maximum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use malloc only when assembly timing information exceeds the former local-string stack threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Retry timing metadata formatting with exact heap storage only when the sensible stack buffer is insufficient. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Route assembly timing heap-buffer cleanup through the same helper as temporary path buffers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Rely on free(nullptr) and name the stack-backed timing buffer explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Eliminate the separate heap pointer and free assembly timing storage only when it differs from the stack buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

simonrozsival · 2026-08-26T19:56:05Z

/review

github-actions · 2026-08-26T19:56:44Z

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12513

github-actions

⚠️ Needs Changes — 1 error.

The new pointer/stack-buffer ownership paths and TMPDIR/file cleanup are sound, but file-based timing output now inherits the logcat size limit and can emit truncated, unparsable records. CI build 1568587 is still in progress; all completed checks are currently passing.

Generated by Android PR Reviewer for #12513 · gpt56 · 287.3 AIC · ⌖ 8.87 AIC · ⊞ 25.7K
_{Comment /review to run again}

`format_message()` formatted every timing record into a fixed `char [MAX_LOGCAT_MESSAGE_LENGTH]` buffer, so a record longer than 1023 bytes was silently truncated. Because `more_info` is formatted *before* the `; elapsed: ...` suffix, a long enough value dropped the timing data itself and produced an unparseable record. `more_info` is unbounded -- `add_more_info (const char*)` uses `strlen()` and is passed managed type names -- and `dump()` is shared by `dump_to_logcat()` and `dump_to_file()`, so file output was capped for no reason. Split the formatting in two, following the existing `format_joined_path()`/`join_paths()` convention: `format_message()` now returns the message length, or the negative required capacity when the buffer is too small, and `build_message()` retries into a `malloc()`ed buffer in that case. Callers free the result only when it differs from the stack buffer, so the common case stays allocation-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e

`FastTiming::add_more_info` had an overload taking a `char (&)[Size]` together with the `int` returned by `snprintf`. That return value is the length the message *would* have had, not the length that was written, so the overload clamped it to `Size - 1` to avoid handing out a length that runs past the end of the buffer. The clamp was correct, but it meant an oversized message was silently truncated, and the three CoreCLR callers each needed a 1023-byte stack buffer to format into. Since `more_info` is stored as a `std::string` anyway, that intermediate buffer bought nothing. All three messages are just an assembly name followed by a constant suffix, so add an overload taking the two parts as `std::string_view`s and building the string directly. The length is then exact by construction, there is no buffer to overflow and nothing to truncate. The last caller of the array-size overload is in `monodroid-glue.cc`, where the buffer is exactly sized for a base-10 `size_t` and so cannot truncate. It now passes the length explicitly and asserts that it fits, which lets the array-size overload go away entirely. `add_more_info` implementations now share a `store_more_info` helper, which also takes ownership of the string so it isn't leaked when there is no open timing sequence. No change in libc++ usage (76 undefined references before and after). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e

Copilot AI lite review requested due to automatic review settings August 25, 2026 11:46

simonrozsival requested a review from jonathanpeppers as a code owner August 25, 2026 11:46

Copilot started reviewing on behalf of simonrozsival August 25, 2026 11:46 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread src/native/common/runtime-base/timing-internal.cc Outdated

Comment thread src/native/common/runtime-base/timing-internal.cc

simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Aug 25, 2026

simonrozsival mentioned this pull request Aug 25, 2026

Replace simple CLR local strings #12517

Open

simonrozsival force-pushed the dev/simonrozsival/remove-timing-dynamic-strings branch from 6b89b87 to 1e2809e Compare August 25, 2026 15:06

simonrozsival changed the base branch from main to dev/simonrozsival/use-join-paths-for-logger August 25, 2026 15:10

simonrozsival force-pushed the dev/simonrozsival/remove-timing-dynamic-strings branch 2 times, most recently from 176c7d7 to 75a294a Compare August 25, 2026 15:29

simonrozsival changed the title ~~Use fixed buffers for timing strings~~ Remove local strings from timing Aug 25, 2026

simonrozsival force-pushed the dev/simonrozsival/remove-timing-dynamic-strings branch 9 times, most recently from d9fb4b3 to f02f1d2 Compare August 25, 2026 22:23

simonrozsival force-pushed the dev/simonrozsival/remove-timing-dynamic-strings branch from f02f1d2 to 17bbadb Compare August 26, 2026 07:34

Base automatically changed from dev/simonrozsival/use-join-paths-for-logger to main August 26, 2026 14:38

simonrozsival and others added 7 commits August 26, 2026 16:38

Use fixed buffers for timing strings

15c5db2

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Handle timing log path failures

fac2095

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Remove local strings from timing callers

b6675c8

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Update CoreCLR APK size references

de374d3

The fixed timing buffers reduce libmonodroid.so from 1,184,800 to 1,094,848 bytes. Refresh all four affected CoreCLR package baselines using build 1566719 output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Remove obsolete assembly store strings include

2e0ceba

Assembly store timing metadata no longer uses the local-string types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Preserve full assembly timing names

360f57d

Allocate timing metadata to its exact size instead of treating the former local-string stack threshold as a maximum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Keep short timing metadata on the stack

4751d34

Use malloc only when assembly timing information exceeds the former local-string stack threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

simonrozsival and others added 4 commits August 26, 2026 16:38

Report required timing metadata capacity

64f173b

Retry timing metadata formatting with exact heap storage only when the sensible stack buffer is insufficient. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use shared temporary buffer cleanup

60183e7

Route assembly timing heap-buffer cleanup through the same helper as temporary path buffers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use free directly for timing buffers

d353427

Rely on free(nullptr) and name the stack-backed timing buffer explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use timing pointers for buffer ownership

2a66e35

Eliminate the separate heap pointer and free assembly timing storage only when it differs from the stack buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

simonrozsival force-pushed the dev/simonrozsival/remove-timing-dynamic-strings branch from 17bbadb to 2a66e35 Compare August 26, 2026 14:39

github-actions Bot reviewed Aug 26, 2026

View reviewed changes

Comment thread src/native/common/include/runtime-base/timing-internal.hh

simonrozsival mentioned this pull request Aug 27, 2026

[CoreCLR] Remove libc++ from the CoreCLR host #12533

Open

Navigation Menu

Uh oh!

Remove local strings from timing - #12513

Remove local strings from timing#12513
simonrozsival wants to merge 13 commits into
mainfrom
dev/simonrozsival/remove-timing-dynamic-strings

simonrozsival commented Aug 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

simonrozsival commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

simonrozsival commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Uh oh!

simonrozsival commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

simonrozsival commented Aug 25, 2026 •

edited

Loading

github-actions Bot commented Aug 26, 2026 •

edited

Loading