From 3c61b469a1b06eba694779601fbc91ba2eb732fa Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 01:07:07 -0700 Subject: [PATCH 1/3] ADFA-5574: read each watched process the cheapest correct way 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 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/utils/MemoryUsageWatcher.kt | 72 +++--- .../androidide/utils/ProcessMemoryReader.kt | 225 ++++++++++++++++++ .../MemoryUsageWatcherReaderFallbackTest.kt | 70 ++++++ .../MemoryUsageWatcherSampleAlignmentTest.kt | 5 +- .../utils/ProcessMemoryReaderTest.kt | 122 ++++++++++ 5 files changed, 455 insertions(+), 39 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/utils/ProcessMemoryReader.kt create mode 100644 app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherReaderFallbackTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/utils/ProcessMemoryReaderTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index 3935717d5e..ba6bf746c2 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -17,16 +17,11 @@ package com.itsaky.androidide.utils -import android.app.ActivityManager -import android.os.Debug import android.os.Debug.MemoryInfo import androidx.annotation.VisibleForTesting import androidx.collection.IntObjectMap import androidx.collection.MutableIntObjectMap -import androidx.core.content.getSystemService -import com.itsaky.androidide.app.BaseApplication import com.itsaky.androidide.tasks.cancelIfActive -import com.termux.shared.reflection.ReflectionUtils import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.DelicateCoroutinesApi @@ -61,16 +56,9 @@ class MemoryUsageWatcher private val mainDispatcher: CoroutineContext = Dispatchers.Main.immediate, private val nowMillis: () -> Long = System::currentTimeMillis, // Injectable for the same reason the other watchers' readers are: it is the one part of a - // sample that needs a device. ActivityManager.getProcessMemoryInfo is rate-limited and - // internally uses Debug.getMemoryInfo, so the reflective call goes around the limit. - private val readTotalPssKb: (Int, MemoryInfo) -> Int = { pid, into -> - ReflectionUtils.invokeMethod(android_os_Debug_getMemoryInfo, null, pid, into) - - // From https://developer.android.com/tools/dumpsys#meminfo - // "PSS is a good measure for the actual RAM weight of a process and for comparison - // against the RAM use of other processes and the total available RAM." - into.totalPss - }, + // sample that needs a device. A factory rather than a reader, because which read is correct + // depends on the process -- see [ProcessMemoryReaders] (ADFA-5574). + private val readerFor: (Int) -> ProcessMemoryReader = ProcessMemoryReaders::chooseReader, ) { /** * Milliseconds between samples. Changing it clears the history: the chart reads a sample's @@ -143,19 +131,6 @@ class MemoryUsageWatcher var listener: MemoryUsageListener? = null companion object { - private val android_os_Debug_getMemoryInfo by lazy { - checkNotNull( - ReflectionUtils.getDeclaredMethod( - Debug::class.java, - "getMemoryInfo", - Int::class.javaPrimitiveType, - MemoryInfo::class.java, - ), - ) { - "Unable to find getMemoryInfo method in android.os.Debug class" - } - } - /** * Samples retained per series. * @@ -221,14 +196,6 @@ class MemoryUsageWatcher @VisibleForTesting internal fun readUsages() { if (memoryUsage.isEmpty()) { - // Nothing to sample. Returning before the service lookup keeps an idle watcher off - // BaseApplication, which a unit test does not have. - return - } - - val activityManager = BaseApplication.baseInstance.getSystemService() - if (activityManager == null) { - log.error("ActivityManager is null") return } @@ -248,7 +215,7 @@ class MemoryUsageWatcher } // values are in kB, convert to bytes - sampled += proc to readTotalPssKb(pid, proc.memInfo) * 1024L + sampled += proc to readKb(proc) * 1024L } synchronized(historyLock) { @@ -265,6 +232,27 @@ class MemoryUsageWatcher } } + /** + * This process's footprint in kB, falling back to the reflective read if the cheap one + * fails. + * + * The fallback latches on the process, so a rollup that cannot be read -- the process gone, + * a permission this build does not have -- costs one failed attempt rather than one every + * second for the rest of the session. + */ + private fun readKb(proc: ProcessMemoryInfo): Int { + val kb = proc.reader.totalKb(proc.pid, proc.memInfo) + if (kb != ProcessMemoryReaders.UNAVAILABLE) { + return kb + } + if (proc.reader !== DebugMemoryInfoReader) { + ProcessMemoryReaders.logFallback(proc.pid) + proc.reader = DebugMemoryInfoReader + return proc.reader.totalKb(proc.pid, proc.memInfo) + } + return 0 + } + /** * Watches the memory usage of the given process. * @@ -297,7 +285,7 @@ class MemoryUsageWatcher // of the session. Without this, the exported file could not tell those zeros // from a process that really was using no memory (ADFA-5531). watchedSinceMillis = nowMillis(), - ) + ).also { it.reader = readerFor(pid) } } /** @@ -499,6 +487,14 @@ class MemoryUsageWatcher ) { internal val memInfo: MemoryInfo = MemoryInfo() + /** + * How this process's footprint is read, chosen once when it starts being watched. + * + * Per process rather than per sample: the choice needs a file-existence check, and a + * read that fails at runtime latches here so it is not retried every second. + */ + internal var reader: ProcessMemoryReader = DebugMemoryInfoReader + val usageHistory: ShiftedLongArray get() = _history diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProcessMemoryReader.kt b/app/src/main/java/com/itsaky/androidide/utils/ProcessMemoryReader.kt new file mode 100644 index 0000000000..1580e08977 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/ProcessMemoryReader.kt @@ -0,0 +1,225 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.os.Debug +import android.os.Debug.MemoryInfo +import android.os.Process +import androidx.annotation.VisibleForTesting +import com.itsaky.androidide.BuildConfig +import com.termux.shared.reflection.ReflectionUtils +import org.slf4j.LoggerFactory +import java.io.File + +/** + * Reads one watched process's total memory footprint, in kB (ADFA-5574). + * + * A seam with two implementations, because the processes the metrics carousel plots are not alike + * and reading them all the same way costs the same as reading the most expensive one, three times. + */ +fun interface ProcessMemoryReader { + /** + * This process's footprint in kB, or [ProcessMemoryReaders.UNAVAILABLE] if it could not be + * read. + * + * @param scratch A reusable [MemoryInfo]. Readers that do not need one ignore it; it is a + * parameter rather than an allocation because this runs on every sample. + */ + fun totalKb( + pid: Int, + scratch: MemoryInfo, + ): Int +} + +/** + * Picks the cheapest reader that is still correct for a given process. + * + * The IDE is a Zygote fork and has GPU memory; the tooling server and the Gradle daemon are plain + * OpenJDK processes exec'd from the app's Termux prefix and have none. Measured on a Pixel 6 Pro: + * for the JVMs `smaps_rollup` and `Debug.getMemoryInfo` agree to 0.009% (66,018 against 66,012 kB) + * while the rollup costs 13.4ms against 31.4ms; for the IDE the rollup reads ~124MB low, because + * `dumpsys meminfo` accounts EGL mtrack 89MB and GL mtrack 36MB through the memtrack HAL rather + * than through `/proc/pid/smaps`, where a rollup cannot see them. That is 23% of the IDE's total, + * so the IDE keeps the expensive read. + */ +object ProcessMemoryReaders { + /** Returned when a process's footprint could not be read at all. */ + const val UNAVAILABLE = -1 + + private val log = LoggerFactory.getLogger(ProcessMemoryReaders::class.java) + + /** + * Whether this kernel offers a rollup at all. + * + * Checked once. `smaps_rollup` arrived in Linux 4.14, so Android 10 in practice, and minSdk + * here is 28 -- a device below that gets the reflective read for everything, which is what it + * had before. + */ + @VisibleForTesting + internal val isRollupSupported: Boolean by lazy { + File("/proc/self/smaps_rollup").exists() + } + + /** + * The reader for [pid], decided once when a process starts being watched. + * + * `pid == Process.myPid()` is the whole test, and it costs nothing: the only Zygote-forked + * process the carousel plots is the app itself. Nothing has to inspect `/proc` to find out. + */ + fun chooseReader(pid: Int): ProcessMemoryReader = + chooseReader(pid, Process.myPid(), isRollupSupported).also { chosen -> + if (BuildConfig.DEBUG && chosen === SmapsRollupReader) { + warnIfProcessHasGraphicsMemory(pid) + } + } + + /** + * Complains if a process given the cheap read turns out to be an Android runtime process. + * + * The rule rests on an assumption about the three processes plotted today: only the app's own + * is Zygote-forked, and only a Zygote fork has graphics memory a rollup cannot see. Add a + * fourth watched process that is one, and its line would quietly read about a quarter low -- + * the failure this whole ticket is about, arriving silently. One maps scan when a process starts + * being watched, in debug builds only, turns that into something someone notices. + */ + private fun warnIfProcessHasGraphicsMemory(pid: Int) { + val isRuntimeProcess = + runCatching { + File("/proc/$pid/maps").useLines { lines -> + lines.any { it.contains("libandroid_runtime.so") } + } + }.getOrDefault(false) + if (isRuntimeProcess) { + log.error( + "pid {} maps libandroid_runtime.so, so it may hold graphics memory that " + + "smaps_rollup cannot see. Its memory line will read low. See ADFA-5574.", + pid, + ) + } + } + + @VisibleForTesting + internal fun chooseReader( + pid: Int, + ownPid: Int, + rollupSupported: Boolean, + ): ProcessMemoryReader = + if (pid == ownPid || !rollupSupported) { + DebugMemoryInfoReader + } else { + SmapsRollupReader + } + + internal fun logFallback(pid: Int) { + log.warn("smaps_rollup unreadable for pid {}; falling back to Debug.getMemoryInfo", pid) + } +} + +/** + * `Debug.getMemoryInfo`, reached reflectively. + * + * The only source that includes graphics memory, which is why the app's own process uses it. + * Reflective because `ActivityManager.getProcessMemoryInfo` is rate-limited and internally calls + * this, so going straight to it sidesteps the limit. + */ +object DebugMemoryInfoReader : ProcessMemoryReader { + private val getMemoryInfo: java.lang.reflect.Method by lazy { + checkNotNull( + ReflectionUtils.getDeclaredMethod( + Debug::class.java, + "getMemoryInfo", + Int::class.javaPrimitiveType, + MemoryInfo::class.java, + ), + ) { + "Unable to find getMemoryInfo method in android.os.Debug class" + } + } + + override fun totalKb( + pid: Int, + scratch: MemoryInfo, + ): Int { + ReflectionUtils.invokeMethod(getMemoryInfo, null, pid, scratch) + + // From https://developer.android.com/tools/dumpsys#meminfo + // "PSS is a good measure for the actual RAM weight of a process and for comparison + // against the RAM use of other processes and the total available RAM." + return scratch.totalPss + } +} + +/** + * The kernel's own PSS total, from `/proc/pid/smaps_rollup`. + * + * The `Pss:` field alone, not `Pss` plus `SwapPss`. Measured against `Debug.getMemoryInfo` on a + * JVM process, `Pss` alone was 6kB *higher* out of 66MB, so adding swap would move it further + * away rather than closer. + * + * Cheaper than walking `/proc/pid/smaps` because the kernel does the summation and hands back one + * short file rather than one stanza per mapping -- 22 lines against 93,120 for the IDE. The kernel + * still walks every mapping to compute it, which is why this is 2.3x cheaper and not 40x. + */ +object SmapsRollupReader : ProcessMemoryReader { + override fun totalKb( + pid: Int, + scratch: MemoryInfo, + ): Int = + runCatching { + File("/proc/$pid/smaps_rollup").useLines { lines -> pssKbFrom(lines) } + }.getOrDefault(ProcessMemoryReaders.UNAVAILABLE) + + /** + * Picks the rollup's `Pss` out of [lines] and reads its value. + * + * Separate from [totalKb] so both halves can be tested: choosing the right line matters as much + * as parsing it, and a test that does its own line-picking would pin only the parse. + */ + @VisibleForTesting + internal fun pssKbFrom(lines: Sequence): Int = + lines + .firstOrNull { it.startsWith(PSS_PREFIX) } + ?.let(::firstIntOrUnavailable) + ?: ProcessMemoryReaders.UNAVAILABLE + + /** + * The first run of digits in a line, without allocating. + * + * `Pss: 425176 kB`. Hand-scanned rather than split, because this runs on every + * sample for every watched process. + */ + private fun firstIntOrUnavailable(line: String): Int { + var value = 0 + var seen = false + for (c in line) { + if (c in '0'..'9') { + value = value * 10 + (c - '0') + seen = true + } else if (seen) { + break + } + } + return if (seen) value else ProcessMemoryReaders.UNAVAILABLE + } + + /** + * Deliberately with the colon. The rollup also carries `Pss_Anon`, `Pss_File`, `Pss_Shmem` and + * `Pss_Dirty`, and a prefix of `Pss` alone would match whichever came first. + */ + private const val PSS_PREFIX = "Pss:" +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherReaderFallbackTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherReaderFallbackTest.kt new file mode 100644 index 0000000000..48130182f1 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherReaderFallbackTest.kt @@ -0,0 +1,70 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * What the sampler does when the cheap read fails (ADFA-5574). + * + * A rollup can be unreadable for reasons that are not the kernel's capability -- the process exited + * between being listed and being read, most likely. The sampler must not lose the series over it, + * and must not pay for the failure once a second for the rest of the session. + */ +@RunWith(RobolectricTestRunner::class) +class MemoryUsageWatcherReaderFallbackTest { + private var clock = 1_700_000_000_000L + + @Test + fun `a read that fails latches the process onto the reflective one`() { + var cheapAttempts = 0 + val alwaysUnavailable = + ProcessMemoryReader { _, _ -> + cheapAttempts++ + ProcessMemoryReaders.UNAVAILABLE + } + val watcher = + MemoryUsageWatcher( + nowMillis = { + clock += TICK_MILLIS + clock + }, + readerFor = { alwaysUnavailable }, + ) + watcher.watchProcess(PID, "IDE") + + watcher.readUsages() + watcher.readUsages() + + val proc = checkNotNull(watcher.getMemoryUsage(PID)) + assertThat(proc.reader).isSameInstanceAs(DebugMemoryInfoReader) + + // Once, not once per sample. The latch is the point: without it the sampler would try the + // unreadable file every second and take the failure path every time. + assertThat(cheapAttempts).isEqualTo(1) + } + + private companion object { + const val PID = 4242 + + const val TICK_MILLIS = 1_000L + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt index 5b7e2a21aa..db76f4ee39 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt @@ -39,7 +39,10 @@ class MemoryUsageWatcherSampleAlignmentTest { clock += TICK_MILLIS clock }, - readTotalPssKb = readPssKb, + // The seam is a factory now (ADFA-5574): which read is correct depends on the process. + // These cases are about when values are appended, not how they are obtained, so every + // process gets the same stub. + readerFor = { ProcessMemoryReader { pid, scratch -> readPssKb(pid, scratch) } }, ) @Test diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProcessMemoryReaderTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProcessMemoryReaderTest.kt new file mode 100644 index 0000000000..ded0b8b0ce --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/ProcessMemoryReaderTest.kt @@ -0,0 +1,122 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Which read is used for which process, and what the cheap one makes of a rollup (ADFA-5574). + * + * The equivalence of the two reads is deliberately not asserted here. `Debug.getMemoryInfo` is not + * meaningfully callable off a device, and the interesting part of the claim is a device fact: for a + * plain JVM the rollup agrees with it to 0.009%, while for the app's own process it reads ~124MB low + * because graphics memory is accounted through memtrack rather than through `/proc/pid/smaps`. That + * is recorded on the ticket from a real measurement. What can be pinned here is the rule that acts + * on it, and the parse. + */ +@RunWith(RobolectricTestRunner::class) +class ProcessMemoryReaderTest { + @Test + fun `the app's own process keeps the expensive read`() { + // It is the only Zygote fork the carousel plots, and the only one with GPU memory. A rollup + // cannot see EGL or GL mtrack, so this process would silently lose about a quarter of its + // footprint. + val reader = ProcessMemoryReaders.chooseReader(pid = OWN_PID, ownPid = OWN_PID, rollupSupported = true) + + assertThat(reader).isSameInstanceAs(DebugMemoryInfoReader) + } + + @Test + fun `every other process gets the rollup`() { + // The tooling server and the Gradle daemon: plain OpenJDK processes with no graphics + // memory, where the rollup is the same number for less than half the cost. + val reader = ProcessMemoryReaders.chooseReader(pid = OTHER_PID, ownPid = OWN_PID, rollupSupported = true) + + assertThat(reader).isSameInstanceAs(SmapsRollupReader) + } + + @Test + fun `a kernel without a rollup falls back for everything`() { + // smaps_rollup arrived in Linux 4.14, so Android 10 in practice, and minSdk here is 28. + // Such a device gets exactly what it had before this change. + val reader = ProcessMemoryReaders.chooseReader(pid = OTHER_PID, ownPid = OWN_PID, rollupSupported = false) + + assertThat(reader).isSameInstanceAs(DebugMemoryInfoReader) + } + + @Test + fun `the rollup's own Pss is read, not one of the fields that start like it`() { + // A rollup carries Pss_Anon, Pss_File, Pss_Shmem and Pss_Dirty as well, and matching on + // "Pss" alone would take whichever came first -- here Pss_Dirty, a different number. + val value = + parse( + """ + 02000000-7ffc009000 ---p 00000000 00:00 0 [rollup] + Rss: 653352 kB + Pss_Dirty: 379731 kB + Pss: 441070 kB + Pss_Anon: 385191 kB + SwapPss: 15 kB + """.trimIndent(), + ) + + assertThat(value).isEqualTo(441070) + } + + @Test + fun `a rollup with no Pss line is unavailable rather than zero`() { + // Zero is a measurement -- a process really using no memory. Unavailable is the absence of + // one, and the caller falls back rather than plotting it. + assertThat(parse("Rss: 653352 kB")).isEqualTo(ProcessMemoryReaders.UNAVAILABLE) + } + + @Test + fun `a Pss line with no number is unavailable`() { + assertThat(parse("Pss: kB")).isEqualTo(ProcessMemoryReaders.UNAVAILABLE) + } + + @Test + fun `a process with no rollup at all is unavailable`() { + // The pid is gone, or the kernel has no rollup. Either way this must not throw: it runs on + // the sampling thread once a second. + val value = SmapsRollupReader.totalKb(NO_SUCH_PID, android.os.Debug.MemoryInfo()) + + assertThat(value).isEqualTo(ProcessMemoryReaders.UNAVAILABLE) + } + + /** + * The reader's own line-picking and parsing, over a fixture. + * + * Through [SmapsRollupReader.pssKbFrom], not by finding the line here first: an earlier version + * of this helper did its own `startsWith("Pss:")` and so pinned only the number extraction -- + * loosening the reader's prefix to "Pss" left every case below green. + */ + private fun parse(rollup: String): Int = SmapsRollupReader.pssKbFrom(rollup.lineSequence()) + + private companion object { + const val OWN_PID = 4242 + + const val OTHER_PID = 4243 + + /** Comfortably above any real pid on a device, so `/proc/` cannot exist. */ + const val NO_SUCH_PID = 999_999 + } +} From 63c994b7a13a39cd3da483dbd9813609d63e891f Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 16:33:12 -0700 Subject: [PATCH 2/3] ADFA-5574: chart guards at the size the strip actually gives the plot 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 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsChartLargeTextTest.kt | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsChartLargeTextTest.kt diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartLargeTextTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartLargeTextTest.kt new file mode 100644 index 0000000000..d644802693 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartLargeTextTest.kt @@ -0,0 +1,129 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import com.itsaky.androidide.utils.NetworkUsageWatcher +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * The chart at the size the carousel actually gives it, with the text a low-vision user runs + * (ADFA-5602). + * + * Every other chart test lays out at [CHART_HEIGHT], 400px, which is far taller than the strip: + * `editor_mem_usage_view_height` is 248dp and the plot is only the part of it left over after the + * title row, the legend and the arrows. At 400px there is room for the axis text to grow and + * nothing collapses, which is why the whole suite passed while the chart on the device drew + * nothing at all at 2x. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsChartLargeTextTest { + private val context = ApplicationProvider.getApplicationContext() + + private fun laidOutChart(height: Int): SafeLineChart { + val chart = SafeLineChart(context) + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L + it }, + LongArray(SAMPLES) { 500L + it }, + LongArray(SAMPLES), + ) + }, + ).attach(chart) + chart.layOutAndDraw(CHART_WIDTH, height) + return chart + } + + /** The x labels the axis would draw, as the reader sees them. */ + private fun xLabels(chart: SafeLineChart): List { + val axis = chart.xAxis + val formatter = axis.valueFormatter ?: return emptyList() + return axis.mEntries.map { formatter.getFormattedValue(it, axis).orEmpty() } + } + + @Test + fun `the plot keeps a usable area in the strip the carousel gives it`() { + val chart = laidOutChart(STRIP_PLOT_HEIGHT) + + val handler = chart.viewPortHandler + assertWithMessage("content width").that(handler.contentWidth()).isGreaterThan(0f) + assertWithMessage("content height").that(handler.contentHeight()).isGreaterThan(0f) + } + + @Test + @Config(fontScale = 2.0f) + fun `the plot keeps a usable area at 2x font scale`() { + // The strip's height is fixed, so everything the axes and the legend reserve comes out of + // the plot. At 2x that reservation grew past what was there. + val chart = laidOutChart(STRIP_PLOT_HEIGHT) + + val handler = chart.viewPortHandler + assertWithMessage("content width").that(handler.contentWidth()).isGreaterThan(0f) + assertWithMessage("content height").that(handler.contentHeight()).isGreaterThan(0f) + } + + @Test + @Config(fontScale = 2.0f) + fun `the time axis still says how long ago, not 'now' for every label`() { + // The reported symptom. ElapsedTimeFormatter answers "now" whenever a label's value equals + // the axis maximum, so an axis whose range has collapsed labels every tick "now" -- and the + // same collapse is why nothing is drawn. + val chart = laidOutChart(STRIP_PLOT_HEIGHT) + + val labels = xLabels(chart) + assertThat(labels).isNotEmpty() + assertWithMessage("labels were $labels").that(labels.any { it != "now" }).isTrue() + } + + @Test + fun `a series with no readings at all does not collapse the time axis`() { + // What the device showed when this was reported: the legend read "Power - n/a", the plot was + // empty, and every x label read "now". A power source that stops answering gives the chart a + // series of pure sentinels, which is not the same as no chart at all -- the axis still has to + // say how long ago each sample was. + val chart = SafeLineChart(context) + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage(LongArray(0), LongArray(0), LongArray(0)) + }, + ).attach(chart) + chart.layOutAndDraw(CHART_WIDTH, STRIP_PLOT_HEIGHT) + + val labels = xLabels(chart) + assertWithMessage("labels were $labels, xRange=${chart.xAxis.mAxisMinimum}..${chart.xAxis.mAxisMaximum}") + .that(labels.all { it == "now" } && labels.isNotEmpty()) + .isFalse() + } + + private companion object { + const val SAMPLES = 200 + + /** + * What the plot gets inside the 248dp strip once the title row, legend and arrows have + * taken theirs. Robolectric's density is 1.0, so dp and px are the same here. + */ + const val STRIP_PLOT_HEIGHT = 150 + } +} From 37a2a80c9bd91b4d113be0db7f9c09cc25f7838f Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 17:38:04 -0700 Subject: [PATCH 3/3] ADFA-5574: take the carousel's last English words out of the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MemoryUsageChartRenderer.kt | 8 ++-- .../androidide/ui/MetricsChartRenderer.kt | 6 ++- .../ui/NetworkUsageChartRenderer.kt | 47 ++++++++++++++----- .../androidide/ui/PowerUsageChartRenderer.kt | 32 ++++++++----- resources/src/main/res/values/strings.xml | 4 ++ 5 files changed, 67 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt index a313351855..a7fe80d1b4 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt @@ -17,6 +17,7 @@ package com.itsaky.androidide.ui +import android.content.Context import androidx.annotation.UiThread import androidx.collection.IntObjectMap import androidx.collection.MutableIntIntMap @@ -107,7 +108,7 @@ class MemoryUsageChartRenderer( setDrawCircleHole(false) setDrawValues(false) isHighlightEnabled = false - label = labelFor(proc.pname, entries.lastOrNull()?.y ?: 0f) + label = labelFor(chart.context, proc.pname, entries.lastOrNull()?.y ?: 0f) } } @@ -182,7 +183,7 @@ class MemoryUsageChartRenderer( dataset.entries[index].y = proc.usageHistory.megabytesAt(index) } - dataset.label = labelFor(proc.pname, dataset.entries.lastOrNull()?.y ?: 0f) + dataset.label = labelFor(chart.context, proc.pname, dataset.entries.lastOrNull()?.y ?: 0f) dataset.notifyDataSetChanged() dataChanged = true } @@ -218,9 +219,10 @@ class MemoryUsageChartRenderer( } private fun labelFor( + context: Context, pname: String, megabytes: Float, - ): String = "%s - %.2fMB".format(pname, megabytes) + ): String = context.getString(R.string.metrics_legend_entry, pname, "%.2fMB".format(megabytes)) } internal const val BYTES_PER_MEGABYTE = 1024.0 * 1024.0 diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index a2202c154a..8ba64dbabe 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -376,7 +376,8 @@ abstract class MetricsChartRenderer( // chooser -- so one gesture both undocked the strip and cleared every buffer. onSecondPointerDown = { axisTapListener?.abandonGesture() } - xAxis.valueFormatter = ElapsedTimeFormatter(sampleIntervalMillis) + xAxis.valueFormatter = + ElapsedTimeFormatter(sampleIntervalMillis, context.getString(R.string.metrics_axis_now)) // One label per 15 samples keeps the window readable without crowding. xAxis.granularity = X_LABEL_GRANULARITY_SAMPLES xAxis.isGranularityEnabled = true @@ -656,6 +657,7 @@ abstract class MetricsChartRenderer( */ private class ElapsedTimeFormatter( private val sampleIntervalMillis: () -> Long, + private val nowLabel: String, ) : IAxisValueFormatter { override fun getFormattedValue( value: Float, @@ -663,7 +665,7 @@ abstract class MetricsChartRenderer( ): String { val newestIndex = (axis?.mAxisMaximum ?: value) val secondsAgo = ((newestIndex - value) * sampleIntervalMillis() / 1000f).roundToLong() - return if (secondsAgo <= 0L) "now" else "-%ds".format(secondsAgo) + return if (secondsAgo <= 0L) nowLabel else "-%ds".format(secondsAgo) } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt index eeb6d833de..63113b946d 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt @@ -17,6 +17,7 @@ package com.itsaky.androidide.ui +import android.content.Context import android.graphics.Color import androidx.annotation.UiThread import com.github.mikephil.charting.components.AxisBase @@ -29,7 +30,6 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.NetworkUsageWatcher import com.itsaky.androidide.utils.NetworkUsageWatcher.NetworkUsage -import java.util.Locale import kotlin.math.ceil import kotlin.math.log10 import kotlin.math.max @@ -77,8 +77,18 @@ class NetworkUsageChartRenderer( val datasets = arrayOf( - dataset(usage.received, chart.context.getString(R.string.metrics_network_received), RECEIVED_COLOR), - dataset(usage.transmitted, chart.context.getString(R.string.metrics_network_transmitted), TRANSMITTED_COLOR), + dataset( + chart.context, + usage.received, + chart.context.getString(R.string.metrics_network_received), + RECEIVED_COLOR, + ), + dataset( + chart.context, + usage.transmitted, + chart.context.getString(R.string.metrics_network_transmitted), + TRANSMITTED_COLOR, + ), ) setData(chart, datasets) { applyAxisRange(it, usage) } @@ -108,13 +118,19 @@ class NetworkUsageChartRenderer( return } - update(received, usage.received, chart.context.getString(R.string.metrics_network_received)) - update(transmitted, usage.transmitted, chart.context.getString(R.string.metrics_network_transmitted)) + update(chart.context, received, usage.received, chart.context.getString(R.string.metrics_network_received)) + update( + chart.context, + transmitted, + usage.transmitted, + chart.context.getString(R.string.metrics_network_transmitted), + ) redraw(chart) { applyAxisRange(it, usage) } } private fun dataset( + context: Context, samples: LongArray, label: String, lineColor: Int, @@ -134,10 +150,11 @@ class NetworkUsageChartRenderer( setDrawCircleHole(false) setDrawValues(false) isHighlightEnabled = false - this.label = labelFor(label, samples.lastOrNull() ?: 0L) + this.label = labelFor(context, label, samples.lastOrNull() ?: 0L) } private fun update( + context: Context, dataset: LineDataSet, samples: LongArray, label: String, @@ -145,7 +162,7 @@ class NetworkUsageChartRenderer( for (index in samples.indices) { dataset.entries[index].y = samples[index].toLogBytes() } - dataset.label = labelFor(label, samples.lastOrNull() ?: 0L) + dataset.label = labelFor(context, label, samples.lastOrNull() ?: 0L) dataset.notifyDataSetChanged() } @@ -158,9 +175,15 @@ class NetworkUsageChartRenderer( * throughput fivefold, with the axis agreeing. */ private fun labelFor( + context: Context, label: String, bytes: Long, - ): String = "%s - %s/s".format(label, formatBytes(bytesPerSecond(bytes), decimals = 1)) + ): String = + context.getString( + R.string.metrics_legend_entry, + label, + "%s/s".format(formatBytes(bytesPerSecond(bytes), decimals = 1)), + ) /** A per-interval byte count as a per-second rate. */ private fun bytesPerSecond(bytes: Long): Double = bytes.toDouble() * MILLIS_PER_SECOND / sampleInterval().coerceAtLeast(1L) @@ -265,9 +288,9 @@ private fun formatBytes( ): String { val clamped = bytes.coerceAtLeast(0.0) return when { - clamped < 1_000 -> "%d B".format(Locale.US, clamped.roundToLong()) - clamped < 1_000_000 -> "%.${decimals}f kB".format(Locale.US, clamped / 1_000) - clamped < 1_000_000_000 -> "%.${decimals}f MB".format(Locale.US, clamped / 1_000_000) - else -> "%.${decimals}f GB".format(Locale.US, clamped / 1_000_000_000) + clamped < 1_000 -> "%d B".format(clamped.roundToLong()) + clamped < 1_000_000 -> "%.${decimals}f kB".format(clamped / 1_000) + clamped < 1_000_000_000 -> "%.${decimals}f MB".format(clamped / 1_000_000) + else -> "%.${decimals}f GB".format(clamped / 1_000_000_000) } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index f83ee47705..bdd73cf251 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -17,6 +17,7 @@ package com.itsaky.androidide.ui +import android.content.Context import android.graphics.Color import androidx.annotation.UiThread import androidx.core.graphics.ColorUtils @@ -82,6 +83,7 @@ class PowerUsageChartRenderer( val datasets = arrayOf( series( + context = context, values = usage.temperatureMilliCelsius, label = context.getString(R.string.metrics_power_temperature), lineColor = TEMPERATURE_COLOR, @@ -89,6 +91,7 @@ class PowerUsageChartRenderer( transform = ::milliCelsiusToCelsius, ), series( + context = context, values = usage.powerMicroWatts, label = context.getString(R.string.metrics_power_draw), lineColor = POWER_COLOR, @@ -126,6 +129,7 @@ class PowerUsageChartRenderer( val context = chart.context update( + context = context, dataset = temperature, values = usage.temperatureMilliCelsius, label = context.getString(R.string.metrics_power_temperature), @@ -133,6 +137,7 @@ class PowerUsageChartRenderer( transform = ::milliCelsiusToCelsius, ) update( + context = context, dataset = power, values = usage.powerMicroWatts, label = context.getString(R.string.metrics_power_draw), @@ -146,6 +151,7 @@ class PowerUsageChartRenderer( /** Rewrites one series' values in place and refreshes its legend entry. */ private fun update( + context: Context, dataset: LineDataSet, values: LongArray, label: String, @@ -155,7 +161,7 @@ class PowerUsageChartRenderer( for (index in values.indices) { dataset.entries[index].y = transform(values[index]) } - dataset.label = labelFor(label, values.lastOrNull(), axis) + dataset.label = labelFor(context, label, values.lastOrNull(), axis) dataset.notifyDataSetChanged() } @@ -259,6 +265,7 @@ class PowerUsageChartRenderer( } private fun series( + context: Context, values: LongArray, label: String, lineColor: Int, @@ -276,24 +283,23 @@ class PowerUsageChartRenderer( setDrawCircleHole(false) setDrawValues(false) isHighlightEnabled = false - this.label = labelFor(label, values.lastOrNull(), axis) + this.label = labelFor(context, label, values.lastOrNull(), axis) } private fun labelFor( + context: Context, label: String, latest: Long?, axis: YAxis.AxisDependency, ): String { val value = latest ?: PowerUsageWatcher.UNAVAILABLE - if (value == PowerUsageWatcher.UNAVAILABLE) { - return "%s - n/a".format(label) - } - - return if (axis == YAxis.AxisDependency.LEFT) { - "%s - %.1fC".format(label, milliCelsiusToCelsius(value)) - } else { - "%s - %s".format(label, formatPower(value)) - } + val reading = + when { + value == PowerUsageWatcher.UNAVAILABLE -> context.getString(R.string.metrics_value_unavailable) + axis == YAxis.AxisDependency.LEFT -> "%.1f\u00b0".format(milliCelsiusToCelsius(value)) + else -> formatPower(value) + } + return context.getString(R.string.metrics_legend_entry, label, reading) } /** @@ -318,7 +324,7 @@ class PowerUsageChartRenderer( // Integer labels need integer grid lines, exactly as the watt axis below does. Now that // the range is tight -- 29 to 33 rather than 0 to 36 -- the axis would otherwise place - // lines half a degree apart and "%dC" would print 29C, 30C, 30C, 31C, 31C. + // lines half a degree apart and the integer format would print 29, 30, 30, 31, 31. chart.axisLeft.granularity = 1f chart.axisLeft.isGranularityEnabled = true @@ -327,7 +333,7 @@ class PowerUsageChartRenderer( override fun getFormattedValue( value: Float, axis: AxisBase?, - ): String = "%dC".format(value.roundToLong()) + ): String = "%d\u00b0".format(value.roundToLong()) } // Watts, not milliwatts: a build peaks in single digit watts, so mW labels spent three diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 9ec7372fe6..26f3208492 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1700,6 +1700,10 @@ Couldn\'t save the metrics data. Received Sent + now + n/a + + %1$s - %2$s