Skip to content

perf: eliminate idle rendering and add mobile benchmarks - #2694

Merged
ethicnology merged 8 commits into
developfrom
perf/mobile-performance-audit
Aug 19, 2026
Merged

perf: eliminate idle rendering and add mobile benchmarks#2694
ethicnology merged 8 commits into
developfrom
perf/mobile-performance-audit

Conversation

@ethicnology

Copy link
Copy Markdown
Member

This PR contains the 14 mobile-performance optimization commits rebased onto develop.

It removes idle rendering work, bounds chart activity, instruments startup, serializes stale asynchronous flows, indexes transaction associations, and adds an isolated Android benchmark profile.

The included report documents the audit and the measured results.

Changes

Idle rendering and chart work

  • Stop inactive progress indicators from driving tickers and rendering.
  • Bound the Bitcoin price chart pulse and avoid unnecessary repaint work.

Async correctness and startup

  • Ignore stale Send input and Pay order-poll results.
  • Instrument startup phases without delaying startup, while preserving the current Tor initialization flow.

Transaction and workload performance

  • Add transaction-association indexing and cover large transaction, coin, swap, and refresh workloads.

Benchmarking

  • Add isolated benchmark flavor wiring and the corresponding build checks.

Measured impact on Pixel 6a

  • Home with the chart hidden changed from 60.25 fps and 79.1% CPU to zero frames over 10 seconds and 0.2% CPU after the fixes settled.
  • Send address changed from 60.24 fps and 66.6% CPU to zero frames over 10 seconds and 0.1% CPU after the fixes settled.
  • Wallet detail changed from 57.49–60.28 fps and 75.8–90.0% CPU to zero frames over 10 seconds and 0.1% CPU after the fixes settled.
  • The source-matched profile measurements stayed within the 60 Hz frame budget, with Home chart-hidden UI-frame p50/p90/p99/max of 3.84/5.16/5.95/8.73 ms and Send p50/p90/p99/max of 1.50/2.68/3.03/3.35 ms.
  • The profile APK was measured before the two final non-graphical safeguards for stale Send input and Pay order polling; focused tests cover those final safeguards.
  • The benchmark wallet used an isolated funded testnet fixture and did not open or copy production wallet data.

@ethicnology ethicnology self-assigned this Aug 19, 2026
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 22s —— View job


Code review in progress

  • Gather context and read the PR diff against origin/develop
  • Review changes for correctness bugs, security issues, and regressions (parallel review agents running, verifying candidates as they land)
  • Post findings as inline review comments
  • Post summary

@ethicnology

Copy link
Copy Markdown
Member Author

@claude review?

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 12m 26s —— View job


Code review

  • Gather context and read the PR diff against origin/develop
  • Review changes for correctness, performance, security, and regressions (8 parallel review agents)
  • Verify candidate findings against the code
  • Post review summary

Overall this is a solid, well-tested PR. I verified the get_transactions_usecase bucket/consumed-index rewrite is a faithful O(n) translation of the old firstWhere/remove semantics (match order, duplicate consumption, and leftover filtering all preserved, and the pipeline is materialized with .toList() so side effects run exactly once), the PayBloc in-flight flag has no interleaving gap or deadlock path (flag toggles are synchronous around awaits with finally on every path), the startup tracker adds only synchronous marks, and the benchmark flavor isolation (testnet seed + mainnet toggle lock) is coherent. Findings below, most important first.


🟠 Fade-out freezes the progress bar mid-sweep

lib/core/widgets/loading/fading_linear_progress.dart:49 and packages/bull_ui/lib/src/feedback/bull_fading_linear_progress.dart:61

TickerMode(enabled: isVisible) mutes the LinearProgressIndicator's ticker the instant trigger flips false, but the AnimatedOpacity fade (whose ticker sits above the TickerMode) still runs for widget.duration (default 1000 ms). So on every loading-complete transition, the bar stops dead mid-sweep and fades out as a frozen streak for a full second — visible at every call site (wallet card sync bar, tx syncing indicator, send screen, ~34 total). The steady-state win (no ticker while fully hidden) is real; only the fade-out window regresses. Consider keeping the ticker enabled during the fade and disabling it only once fully hidden (e.g. AnimatedOpacity.onEnd flips a second state flag that feeds TickerMode). The new tests assert intermediate opacity but not that the indicator still animates while visible, so they don't catch this. Fix this →

🟠 The stale-input guard doesn't cover the scan path

lib/features/send/presentation/bloc/send_cubit.dart:250

onScannedPaymentRequest bumps _paymentRequestInputGeneration but never re-checks it, and its continueOnAddressConfirmed()unifiedBip21Prioritization() chain captures state.paymentRequest before await PaymentRequest.parse(...) (send_cubit.dart:2205-2224) and then emits paymentRequest: unconditionally on both the success and catch branches. So: scan a BIP21 QR with a lightning param, and while the parse is in flight paste a different address — onChangedText bumps the generation and emits the new recipient, then the resumed scan flow overwrites it with the stale scanned request. This is the exact stale-overwrite class the fix(send) commit targets, still open on the scan side. A generation check after each await in the scan continuation (or a shared guarded-emit helper — note the cubit now carries two parallel epoch mechanisms, _bitcoinPreviewEpoch and this counter) would close it. Fix this →

🟡 One malformed order can now fail the whole transaction list

lib/features/transactions/application/usecases/get_transactions_usecase.dart:133-191

The old try { swaps.firstWhere(...) } catch (_) { swap = null; } blocks incidentally swallowed exceptions thrown by the match predicates themselves. The new bare for-loops preserve the predicates but not that containment: a throw from _transactionPaysSwapAddress or _transactionCoversOrderAmount (e.g. ConvertAmount.btcToSats on a non-finite server-supplied amount throws UnsupportedError from .round()) now propagates to the outer catch and turns the entire fetch into TransactionAggregationFailure. Previously that record just rendered unlinked. A per-transaction (or per-predicate) guard restores graceful degradation.

🟡 make android FLAVOR=benchmark fails after the full container build

makefile:246

The FLUTTER_BUILD override fires on FLAVOR == benchmark alone (forcing --profile), but MODE/CONTAINER_OUTPUT switch to profile only when benchmark is a make goal (makefile:197). The variable form make android FLAVOR=benchmark therefore builds app-benchmark-profile.apk inside the container while container cp looks for app-benchmark-debug.apk — a multi-minute build ending in "no such file". Relatedly, the aab $(error) guard and the FORMAT := apk force overlap inconsistently (CLI FORMAT=aab errors, env FORMAT=aab is silently rewritten); picking one mechanism would simplify. The goal form make android benchmark works correctly.

🟡 Benchmark mainnet lock: silent no-op, and only at the presentation layer

lib/features/settings/presentation/bloc/settings_cubit.dart:97

Two aspects worth tightening for a flavor whose whole point is "never mainnet": (1) the only caller walks the user through the "Switch to Mainnet?" warning sheet, whose confirm then silently does nothing — no log, no feedback, the switch just snaps back; (2) the guard lives only in toggleTestnetMode, while SetEnvironmentUsecase/SettingsRepository.setEnvironment stay unguarded, so any other writer (dev menu, integration tests, future settings restore) bypasses it, and a pre-populated DB that already says mainnet is never clamped (the seed guard only runs at DB creation). Enforcing the invariant in the usecase/repository closes all paths.

🔵 Minor

  • Startup tracker drops all marks on retry paths (lib/core/utils/startup_phase_tracker.dart:37): the monotonic phase.index <= _lastPhase.index guard means the keychain-locked retry and the legacy-gate re-run of AppStartupStarted emit zero timeline events (everything is ≤ the first pass's high-water mark, e.g. terminalResolved), so the most interesting startup passes to profile record nothing — and appending a new enum value in the wrong position silently drops later marks. A Set<StartupPhase> seen-set (or per-run reset) avoids the ordering fragility.
  • Chart pulse never restarts on selection (lib/features/bitcoin_price/ui/price_chart_widget.dart:344): forward() runs once in initState; didUpdateWidget restarts only _dotPositionController, so after the first 1.5 s the selected dot is permanently static. The audit doc (WP2) describes the pulse as "entrance and/or selection" — if selection feedback was intended, add a _pulseAnimationController.forward(from: 0) where the selection changes. Also, _dotPositionAnimation is merged into the painter's repaint: listenable but paint() never reads it — each selection change buys 300 ms of repaints that draw pixel-identical output; it can be dropped from the merge (or actually used).
  • PayBloc hand-rolled serialization (lib/features/pay/presentation/pay_bloc.dart:122): _isOrderRequestInFlight + _pendingOrderStatusUpdateId + the while(true) drain (including replaying an update through the poll event's emitter in its finally) reimplement what bloc_concurrency provides declaratively — and this file already uses droppable(). Routing both events through a shared supertype registered with sequential() would delete the three mutable fields and the cross-handler emitter reuse. It works as written (I traced the paths), but every future handler that touches order state must know about the flag.
  • Reuse nits: main.dart:389 still hardcodes appFlavor != 'beta' instead of the new BuildFlavor; the five hand-rolled index-bucket loops and two queue/next-pointer trackers in get_transactions_usecase.dart could collapse to a small helper (and the swap consumption tracker to a Set of swap.id, avoiding deep freezed hashing); the TickerMode fix + ~100-line test suite landed twice because FadingLinearProgress remains a byte-for-byte fork of BullFadingLinearProgress.

Verified clean

  • Transactions refactor: predicate clauses, match order, duplicate consumption, and leftover filtering all reproduce the old semantics; no laziness double-count (.toList() before the consumed sets are read).
  • App startup bloc restructure is truth-table identical for the legacy-gate combinations; tracker plumbing is optional everywhere and adds no awaits.
  • Benchmark gradle flavor (dimension 'channel', debug signing, .benchmark suffix) is wired consistently; appFlavor is a compile-time const so seeding is isolate-safe; beta continues to seed mainnet.
  • Pay order-poll stale checks (requestedOrderId recheck after fetch) and the send onChangedText guard are correct as far as they go.

@ethicnology
ethicnology force-pushed the perf/mobile-performance-audit branch from 928f4c3 to 72f26b3 Compare August 19, 2026 18:17
@ethicnology

Copy link
Copy Markdown
Member Author

@claude re-review please

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 53s —— View job


Re-review

  • Diff the rebased branch against origin/develop and identify what changed since the previous review
  • Check each previous finding against the current code
  • Fresh review pass over the updated diff
  • Post updated review

The branch was rebased since my last review: the four benchmark-flavor commits (makefile, gradle, settings cubit, benchmark seeding in main.dart) were dropped, and fixes for the two 🟠 findings plus the malformed-order 🟡 were folded into the remaining 12 commits, each with new test coverage. I read every changed hunk of the delta. The fixes are correct; I found one new issue introduced by the stale-generation bail (below), plus a few carried-over nits.


Previous findings — status

Finding Status
🟠 Fade-out froze the progress bar mid-sweep Fixed. Both widgets now keep the ticker enabled through the fade and disable it via AnimatedOpacity.onEnd only when fully hidden (lib/core/widgets/loading/fading_linear_progress.dart:47-56, mirrored in bull_ui). I checked the edge cases — cold-start hidden, rapid false→true retarget mid-fade, re-enable before fade-out ends — all handled, and the new tests cover exactly the regression I flagged (ticker enabled at mid-fade, disabled after, not disabled when re-triggered mid-fade).
🟠 Stale-input guard didn't cover the scan path Fixed (with one side effect, see new finding). The generation is now captured in continueOnAddressConfirmed and re-checked after unifiedBip21Prioritization, and unifiedBip21Prioritization re-checks after await _parsePaymentRequest(...) and in its catch (send_cubit.dart:320-327, 2209-2227). The injectable parser enables the new test that reproduces the exact scan-then-paste race I described, and it passes semantically: the stale bolt11 can no longer overwrite the newer recipient.
🟡 One malformed order could fail the whole list Fixed. _transactionCoversOrderAmount now returns false for non-finite amounts before btcToSats (get_transactions_usecase.dart:344), with a test (double.infinity payin) asserting the rest of the list still associates. I re-checked the remaining predicates for throw paths: _transactionPaysSwapAddress is pure set/pattern code with an exhaustive switch, and VM double.round() on huge finite values saturates rather than throws — so the predicates are now total and the old graceful degradation is fully restored.
🟡 make android FLAVOR=benchmark broke; 🟡 benchmark mainnet lock gaps Moot — the benchmark commits are no longer in this PR.
🔵 Startup tracker drops marks on retry Resolved by design. The keychain retry now has a dedicated test showing the retry pass continues emitting its new phases past the first pass's high-water mark ("keychain retry continues phases without duplicates"), so the monotonic guard is intentional and documented. The enum-order fragility remains, but it's now pinned by tests.
🔵 Chart pulse semantics Improved. The pulse is now an explicit one-shot entrance animation ending at scale 1.0 (TweenSequence 0.8→1.2→1.0), removing the frozen-at-arbitrary-scale ambiguity, and the painter refactor (Animation objects + repaint: listenable instead of AnimatedBuilder) is sound — shouldRepaint correctly delegates animation-driven repaints to the listenable. The widget test asserts the key perf claim directly: zero transient callbacks / no scheduled frame once settled, and re-settling after a selection tap.

🟡 New: stale-generation bail leaves loadingBestWallet stuck true

lib/features/send/presentation/bloc/send_cubit.dart:325-327

continueOnAddressConfirmed emits loadingBestWallet: true, then the new guard returns early when the generation changed — without clearing it. Every other terminal path in the function (and its catch) resets the flag; the bail paths are the only ones that don't, and nothing else ever clears it (onChangedText doesn't touch it). In exactly the scenario the fix targets — scan a BIP21+lightning QR, paste a new address while the parse is in flight — the user is left with:

  • the Continue button permanently disabled (send_screen.dart:187 gates on loadingBestWallet), so the newly pasted recipient can't be confirmed, and
  • the send screen's own FadingLinearProgress trigger stuck true (send_screen.dart:103), i.e. a permanently running progress ticker — the exact idle-rendering cost this PR exists to eliminate.

Recovery requires re-scanning or leaving the screen. The new "ignores a stale scan after a newer paste" test asserts recipient/step but not loadingBestWallet, so it doesn't catch this. Cleanest fix: clear loadingBestWallet on the stale bail (or reset it where onChangedText/onScannedPaymentRequest bump the generation, which avoids a stale flow clearing a newer in-flight one), and extend that test to assert the flag. Fix this →

🔵 Carried over, unchanged

  • _dotPositionAnimation is still merged into the painter's repaint: listenable but never read in paint() (price_chart_widget.dart:354-358) — each selection change still buys ~300 ms of pixel-identical repaints. Small, but it's the one remaining bit of dead repaint work in a PR about dead repaint work.
  • PayBloc's hand-rolled serialization (_isOrderRequestInFlight + pending-id drain loop) is unchanged; it works (the new tests cover the stale-poll and shared-in-flight cases well), but bloc_concurrency.sequential() over a shared event supertype would still delete the mutable fields.
  • FadingLinearProgress remains a byte-for-byte fork of BullFadingLinearProgress — the fix and its ~195-line test suite landed twice.

Note

The PR description is now stale: it still says "14 commits" and describes the benchmark flavor wiring, makefile checks, and the profile-APK methodology, none of which are in this diff anymore (12 commits, no makefile/gradle/settings changes). Worth updating so the merge record matches the content.

Verified clean in this pass

  • Startup tracker: marks are all synchronous, mark() swallows sink exceptions, workmanagerCancelled accurately names what initWorkmanager does (cancelAll), and the bloc's phase sequence assertions pin the happy path and the keychain-locked pause.
  • Transactions: the new _CountingList test pins the O(n) access bound; the order-vs-orderSwap independent consumption test closes the semantics I traced last time.
  • Chart painter refactor: repaint listenable created once, painter recreation on rebuild is conservative-but-correct, no listener leak.

@ethicnology
ethicnology force-pushed the perf/mobile-performance-audit branch from 72f26b3 to dd28663 Compare August 19, 2026 18:50
@ethicnology
ethicnology merged commit c74bc0b into develop Aug 19, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant