ADFA-5574: read each watched process the cheapest correct way - #1808
Conversation
The carousel plots three processes and read all three with Debug.getMemoryInfo, which walks every mapping in /proc/pid/smaps. That costs the same as reading the most expensive one, three times. The two kinds of process are not alike. The IDE is a Zygote fork with GPU memory. The tooling server and the Gradle daemon are plain OpenJDK processes exec'd from the app's Termux prefix, parented to the IDE, with no boot.art, no libandroid_runtime and no libart mapped at all. Measured on a Pixel 6 Pro, in-process, 200 iterations, median: getMemoryInfo 31.4ms, smaps_rollup 13.4ms, status VmRSS 0.1ms. For a JVM the rollup agrees with getMemoryInfo to 0.009% -- 66,018 against 66,012 kB, stable over three runs. For the IDE it reads about 124MB low, because dumpsys accounts EGL mtrack 89MB and GL mtrack 36MB through the memtrack HAL rather than through smaps, where a rollup cannot see them. That is 23% of the IDE's total, so the IDE keeps the expensive read and only it does. pid == Process.myPid() is the whole test and costs nothing: the only Zygote-forked process the carousel plots is the app itself. Nothing inspects /proc to decide. The choice is made once, when a process starts being watched, and lives on ProcessMemoryInfo beside the MemoryInfo scratch it already holds. Per sample it would mean a file-existence check every second, and the runtime fallback would have nowhere to latch. A rollup that cannot be read -- the process exited, a permission this build lacks -- costs one failed attempt and then that process uses the reflective read for the rest of the session. The injectable seam changes shape rather than disappearing, from readTotalPssKb to readerFor, so it is still one seam and still injectable. VmRSS is deliberately not used. It would take the read to 0.1ms, but it is about 4% high on the JVMs and it is not additive across processes, which is the property that lets the three lines be summed. Rollup gives the same number as today for less than half the cost. Also removes the ActivityManager lookup in readUsages. It has been dead since the switch to the reflective read -- the constructor's own comment says why the reflective call exists -- but its null check was not: had getSystemService returned null, the sampler would have taken no sample at all, for a service it does not use. Scope, honestly: this is about 94ms/sec of CPU down to about 57. The IDE's own read is 31 of that 57, so no read strategy gets below ~31 while the IDE is sampled at 1Hz. Taking it to zero when nobody is looking is ADFA-5570's visibility gate, not this. What is tested: the rule, both halves of the parse, and the fallback latch. Removing the latch fails its test with two cheap attempts instead of one. Loosening the Pss prefix to "Pss" fails the parse test with 379,731 -- Pss_Dirty -- instead of 441,070; the first draft of that test did its own line-picking and so pinned only the number extraction, which is why the reader now exposes pssKbFrom. What is not tested: that the two reads agree. Debug.getMemoryInfo is not meaningfully callable under Robolectric, so the equivalence rests on the device measurement above and is recorded as such rather than implied. A debug-only check warns if a process given the cheap read turns out to map libandroid_runtime.so, so a future fourth watched process that does use graphics fails loudly instead of quietly reading a quarter low. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
…-5574-cheapest-correct-memory-read
…-5574-cheapest-correct-memory-read
…-5574-cheapest-correct-memory-read
Every other chart test lays out at 400px. The carousel strip is 248dp and the plot is what is left after the title row, the legend and the arrows -- around 150dp. At 400px there is room for the axis text to grow and nothing is ever tight, which is why the whole font-scale suite is green against a chart that was reported blank on a device. These four cases lay out at 150px instead and assert the plot keeps a usable area, at 1.0 and at 2x, and that the time axis does not label every tick "now". They pin nothing about ADFA-5602. They pass before and after, because Robolectric cannot reproduce it: instrumented at this size it reports xLabelWidth=0 and a legend needing 3.0px at 1.0 against 4.5px at 2x, where the device reports 51.5 and 77.3, and its content rect and axis range come out identical at both scales. That measurement is the useful part -- it says why no test here can catch a text-driven layout fault, and it is recorded on ADFA-5602 along with the device numbers that disprove the cause I originally proposed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
An audit of every user-visible string the carousel produces found the split in the wrong place: the nouns were externalised -- page titles, series names, the four build annotations, the dialogs, both error toasts, 25 strings in all -- while the numbers, the units and two actual English words were literals in Kotlin. "now" labelled the x axis whenever a sample was less than half an interval old, and "n/a" stood in for a power reading the device would not give. Both are words rather than symbols, both are on screen, and neither could be translated. They are string resources now, like the labels they sit beside. The legend composed itself in code: "%s - %.2fMB", "%s - %s/s", "%s - %.1fC". A translator got "Battery temp" and never "Battery temp - 27.0C", because the separator and the ordering lived outside any resource -- so a right-to-left locale could not reorder them either. All four renderers build a legend entry through metrics_legend_entry now. The unit symbols stay in code deliberately: MB, kB, GB, B, W, mW, % and /s are international, and a resource per unit would be ceremony without a reader. One screen was using two decimal conventions. NetworkUsageChartRenderer pinned Locale.US in four byte formats while the memory and power pages passed no locale and followed the device, so a German phone showed "1.5 MB" beside "27,0C". The byte formats follow the device now, which is what a user-facing number should do; the tests that assert those strings derive their expectations the same way, so they stay locale-agnostic. Temperature reads "27" with a degree symbol rather than "27C". The symbol is narrower, which matters on an axis inside a 248dp strip, and it marks the number as a temperature rather than leaving a bare C to be read as something else. Written as ° rather than the character, to keep the source ASCII. Not fixed here, and filed separately: all 25 of these strings exist in one locale of fourteen. That is the standing state of every recently added string in this project rather than anything this stack did, and it needs a translation pass rather than a code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
…-5574-cheapest-correct-memory-read
…-5574-cheapest-correct-memory-read
|
Superseded by #1812, which merges all of the carousel work onto current Closing rather than leaving open so review effort is not split: Nothing is lost. This branch is untouched, the commits and their messages are in #1812's history, and reopening is a click if that turns out to be the wrong call. #1812 carries the ticket-by-ticket detail, the five conflict resolutions the ADFA-5514 squash forced — those compile either way, so they are the part worth reviewing hardest — and a device pass on a Pixel 6 Pro covering the daemon plot, the build markers and all three chart pages. |
PR 16 on the carousel stack. Base is
feature/ADFA-5542-cancel-by-build-id(#1807); review that first.Split out of ADFA-5570 as the one part of that plan which stands alone — it changes how a sample is taken, not when or how often, so it needs no change to the chart's x axis and introduces no gaps.
The two kinds of process are not alike
The carousel plots three processes and read all three with
Debug.getMemoryInfo, which walks every mapping in/proc/pid/smaps. But the IDE is a Zygote fork with GPU memory, while the tooling server and the Gradle daemon are plain OpenJDK processes exec'd from the app's Termux prefix — parented to the IDE, with noboot.art, nolibandroid_runtime, nolibartmapped at all.Measured on a Pixel 6 Pro, in-process, 200 iterations, median:
Debug.getMemoryInfoDebug.getMemoryInfo/proc/pid/smaps_rollup/proc/pid/statusVmRSSThe IDE keeps the expensive read because it is the only source that sees graphics memory:
dumpsys meminfoaccounts EGL mtrack 89 MB + GL mtrack 36 MB through the memtrack HAL rather than through smaps, where a rollup cannot see them. That is 23% of the IDE's total. The JVMs have no mtrack/EGL/GL lines at all, which is exactly why they agree to 0.009%.pid == Process.myPid()is the whole test, and it costs nothing: the only Zygote-forked process the carousel plots is the app itself. Nothing inspects/procto decide.Measured on a production build — and it is smaller than I predicted
Release build type, non-debuggable (
flags=[ HAS_CODE ALLOW_CLEAR_USER_DATA LARGE_HEAP ], noDEBUGGABLE), debug-key-signed so it upgrades in place. Same device, same fresh-launch state, editor idle, two 20 s windows each:MemoryUsageWatcA 5.4% reduction in the memory sampler, about 2% of the app's idle CPU. I predicted ~40%, and that prediction was wrong: I derived 94 ms/sec from 3 × 31.4 ms, which assumed every watched process costs what the IDE costs. It does not — the expensive read is expensive in proportion to a process's mapping count, and the IDE has 3,880 of them against a 66 MB JVM's few hundred. The process that must keep the expensive read is the one that dominates it.
Two honest consequences:
I am not going to dress 5.4% up as more than it is. It is a correct change that removes waste with no accuracy loss, and the debug-build numbers oversold it.
Also removed
The
ActivityManagerlookup inreadUsages(). Dead since the switch to the reflective read — the constructor's own comment explains why that read exists — but its null check was not: hadgetSystemServicereturned null, the sampler would have taken no sample at all, for a service it does not use.Tests
the rollup's own Pss is read, not one of the fields that start like it"Pss"gives379,731(Pss_Dirty) instead of441,070a read that fails latches the process onto the reflective onePssline,Pssline with no numberUNAVAILABLE, never0— zero is a measurement, absence is notThe first draft of the parse test did its own line-picking and so pinned only the number extraction — loosening the prefix left it green. That is why the reader now exposes
pssKbFrom, and why the test routes through it.Not tested: that the two reads agree.
Debug.getMemoryInfois not meaningfully callable under Robolectric, so the equivalence rests on the device measurement above and is recorded as such rather than implied. A debug-only check warns if a process given the cheap read turns out to maplibandroid_runtime.so, so a future fourth watched process that does use graphics fails loudly instead of quietly reading a quarter low.Caveats
One device only.
smaps_rollupneeds a Linux 4.14 kernel — Android 10 in practice — and minSdk here is 28, so the fallback matters; it is exercised by forcingrollupSupported = false. Wants a low-end device check: a slower core makes sampling a larger share of the whole, so the Pixel 6 Pro may understate the benefit. That is in the ticket's Steps to QA.Also in this PR: a cross-cutting strings change
Commit
37a2a80c9is not part of ADFA-5574. It is the result of auditing every user-visible string the carousel produces (ADFA-5603), and it sits here because it spans four renderers owned by four different tickets:MetricsChartRenderer.ktMemoryUsageChartRenderer.ktNetworkUsageChartRenderer.ktPowerUsageChartRenderer.ktWhat it changes:
"now"and"n/a"were literals in Kotlin and are now string resources; the legend composed itself in code as"%s - %.2fMB"and now goes throughmetrics_legend_entry, so the separator and ordering are translatable and an RTL locale can reorder them;NetworkUsageChartRendererpinnedLocale.USon four byte formats while the other two pages followed the device, so one screen showed1.5 MBbeside27,0C— the byte formats follow the device now; and temperature reads27°rather than27C.Why it is not on a lower branch. ADFA-5486 cannot host it:
PowerUsageChartRenderer.ktdoes not exist there, arriving with ADFA-5499. The lowest branch holding all four files and on the path to this one is ADFA-5510, which owns none of them — a convenient host, not a correct one. Splitting the change four ways would leave the legend format externalised on some pages and not others at each intermediate commit. It is here as one coherent change instead, deliberately.Unit symbols stay in code: MB, kB, GB, B, W, mW, %, /s and the degree symbol are international, and a resource per unit would be ceremony with no reader.
Reviewing it separately is easy —
git show 37a2a80c9is the whole of it, five files.🤖 Generated with Claude Code
https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j