Navigation performance: library walk, shelf paint, settings heap peak - #54
Open
imshentastic wants to merge 15 commits into
Open
Navigation performance: library walk, shelf paint, settings heap peak#54imshentastic wants to merge 15 commits into
imshentastic wants to merge 15 commits into
Conversation
The QC drain gated its prompt purely on pendingBleQuickConnectPromptStage_ and ignored btManifestPromptAnsweredThisSession_, so answering the prompt at book open and then hitting BT Quick Connect asked the same question again. v187 wired the flag into the edge-detect site only; this completes it. A surviving mismatch implies the earlier answer was 'use my settings', so the drain proceeds to Step 2 with the user's settings intact. Found by a parallel session; the answered-flag is already RTC-backed here (v187 sidecar), so the fix holds across the BT pre-flight silent restarts too.
Both default OFF; with them off the cycle path is byte-for-byte the old behaviour, including tap-detection timing. - sleepCycleDoubleTapBack: a second Power tap within 400ms steps to the previous image. The window is only polled when the setting is on, so the default path adds no delay before deep sleep. Backward displays cursor-2 (the cursor stores the NEXT image), keeping the invariant that back-after-forward returns to the previous picture. - sleepCycleDailyMode: automatic sleeps hold the current image until the calendar day rolls over; manual taps always advance and re-stamp the day. Day key is YYYYMMDD compared for equality, so no epoch math. With no valid clock the gate is inert and per-sleep cycling continues, and enabling the setting without a clock pushes ClockSyncActivity inline (mirrors the Home-clock toggle precedent) so X4 users get NTP right away.
Registering in SettingsList only reaches the flat list and the web settings page; the on-device menu builds a curated tree, so both toggles were invisible on the device. Push them into Display > Sleep Image next to the existing cycle-on-tap row.
checkAutoReconnect calls connectToDevice(), which blocks the main loop 2-3 s when the bonded remote is off or out of range. It is driven by local button presses, so anyone who owns a page-turner but is not using it right now paid that freeze press after press -- a large part of the 'buttons feel slow' reports. Count consecutive failures and hold off 10s/20s/40s/80s/160s, capped at 5 min; a successful connect or any deliberate BT enable clears it, so 'connect now' is never delayed. Also add the timing needed to target the rest: boot->ready (which IS the pause users see, since navigation silent-restarts to defragment) tagged with the restart target, and onEnter duration per activity when it exceeds 150 ms.
Settings was the one common navigation target missing from isLibraryLightBoot, so a Settings-entry restart -- the restart most likely to fire, since the 45 KB free / 30 KB maxAlloc pre-flight trips after a reading session has fragmented the heap -- paid a full LibraryIndex + SeriesIndex + CollectionsStore load that no Settings screen reads. Rebuild Author Keys is the only Settings action that touches the index; it now calls the idempotent begin() first. Without that it would have cleared an empty index and saved it over the user's real one.
Field timing (X4, 35 sleep images, 23 transparent/grey): this scan cost ~1.8 s of a ~6.9 s boot->ready on a navigation silent-restart, on every boot, to reach the same '0 unbaked' answer. Deciding a file is unbakeable means opening and parsing its PNG/BMP header, and an unbakeable file never becomes bakeable, so the work was pure repetition. Cache '<fileCount> <unbaked>' beside the declined sidecar and re-probe only when the sleep dir's file count changes (upload/delete) -- the only way the answer moves on its own. Choosing 'Bake now' drops the cache, since baking changes the verdict without changing the count. The directory listing is still walked every time, so an added or removed file is always noticed.
Field log: 'flushIfDirtyNow SKIPPED: maxAlloc=19444 below hard floor 20480 -- setting change lost across restart'. The floor is correct (it prevents a bad_alloc terminate inside the 102-field JSON build) but the outcome was that a setting the user deliberately changed silently vanished. The deep-sleep path already solves this by lending the framebuffer's ~48 KB to the write. Do the same on the silent-restart path, then restore the buffer so the seamless snapshot still runs. Freeing and immediately re-taking the same block makes the realloc very likely to succeed; if it fails, the snapshot's existing null-guard skips the repaint -- losing a cosmetic no-flash transition beats losing the setting. One hook covers all 23 restart sites.
…walks Two problems in one area, found by adding PERF timing around the SD walk. Reuse across silent restarts. LibraryIndex::ensureWalked() is gated by walkPerformed, a plain RAM bool that resets every reboot, so a silent restart re-walked the card it had walked seconds earlier (~1.0-1.2 s on a 43-book library). An RTC-backed marker now mirrors walkPerformed and is honoured only for a clean silent restart with a loaded on-disk index; cold boot, power-cycle and crash restarts always walk, so a swapped SD card is never trusted. The hazard is a book appearing or disappearing before the restart without the index learning about it -- the walk is how new books are discovered, so a stale marker would hide a just-uploaded book until a power cycle. The marker is armed only by a completed rescan() and cleared by markStale() / forgetPath() / releaseMemory(). Arming is then latched off for the rest of the boot by CrossPointWebServer::begin(), which is the real chokepoint: the server's WS upload, HTTP upload, file-manager delete/rename and every WebDAV write verb change the card directly, and two separate activities start one. Calibre Connect in particular never touched LibraryIndex at all and silent-restarts on exit, so a per-endpoint audit would have to stay exhaustive forever; gating at begin() covers current and future paths by construction. Calibre Connect also now markStale()s on exit, matching File Transfer. Concurrent walks. The same PERF line showed two walks per cold boot with overlapping intervals -- [4671,5875] and [5552,6532]. Activity::render() runs on the ActivityManagerRender task while Activity::loop() runs on the main task holding no RenderLock, and HomeActivity reaches the index from both: loop() calls cachedShelfPaths() to size the shelf for navigation, render() calls it to paint. Both land in resolveBookPaths -> ensureWalked(), whose guard was a check-then-set spanning the entire ~1.2 s walk, so both tasks passed it. The cost was the lesser issue. rescan() builds a map of string_views into the live pathPool and then swaps entries/pathPool/authorKeyPool wholesale, so a concurrent rescan or releaseMemory() leaves the other task's views dangling -- a plausible contributor to the unexplained reboots around Home. walkPerformed is now atomic and a mutex serialises ensureWalked, rescan, releaseMemory and forgetPath, with an unlocked fast path so steady-state renders stay lock-free. markStale() is deliberately left unlocked; the reasoning is in the code. Measured on an X4, 43 books: cold boot 2 walks -> 1, and a Home-target silent restart skips the walk entirely.
Every check in the loadShelfCovers loop exists to decide whether a cover thumbnail needs generating, but on a settled library the thumbnail is already on SD and none of them matter. Ordered as it was, an already-cached cell paid four SD lookups before reaching the one that answers the question: Storage.exists(bookPath) stat 1 CoverThumbStatus::isMarkedFailed() stat 2 (marker file) Epub(bookPath, ...) constructor stat 3 (legacy-cache migration) Storage.exists(resolved) stat 4 (the actual answer) The constructor stat is the non-obvious one -- Epub::Epub runs migrateLegacyCachePath, which stats the card. SD path lookups are the expensive primitive on this stack, and v316's shelfCoversLoaded gate means this pass runs once per boot: on the first Home paint, the one the user waits on. Probe the resolved thumbnail directly via a new SD-free Epub::thumbBmpPathForDimensions(), and fall through to the unchanged sequence on a miss, so generation behaviour -- including the migration the constructor performs -- is untouched. EPUB only. Xtc::getThumbBmpPath() emits a HEIGHT-only template, which sends UITheme::getCoverThumbPath down a legacy-fallback branch that stats SD itself; not reproducible without duplicating that logic, and XTC is rare enough not to be worth the coupling. Measured on an X4, 43 books: the pass now reports 42-48 ms with all four visible cells answered by the single-stat probe.
The shelf cell BMPs are already stored at exactly cell size, so the cost of painting them was never scaling -- it was the SD open plus a Bitmap decode through the renderer's LRU image cache, which refuses to allocate when maxAlloc is low. On a cold boot the library walk fragments the heap just before the shelf paints, so the cache refused and every cell fell back to a direct streamed decode. That is why the shelf stage measured 1563 ms on a cold boot against 306 ms on a silent restart: it was tracking heap, not cover count. Reuse the existing CoverTiles mechanism under a new kRoleShelfCell. A .tile read needs no decode and no cache budget, which removes the heap coupling rather than just making it cheaper. Its own role rather than reusing kRoleCenterThumb because the shelf aspect-FILLs with crop where the centre thumb aspect-FITs -- same header dimensions, different pixels. Perspective params are unused and passed as 0, and the format is unchanged, so no version bump (the header comment already anticipated new roles). Same three-step the carousel centre thumb uses: load, else render into a packed 2bpp buffer via renderCachedBitmapToPacked2bpp / renderBitmapToPacked2bpp, blit, and save. One reusable 3750-byte buffer for the whole cell run, allocated nothrow -- a null buffer just means every cell takes the old decode path. Tiles live beside their .bmp inside the book's cache directory, so an existing cache clear removes them too. Also switch CoverTiles::loadTile from openFileForRead to Storage.open(). A missing tile is the normal "not baked yet" outcome, but openFileForRead routes through the SDK's SDCardManager, which unconditionally Serial.printf()s "File does not exist" with no log level -- four lines per shelf paint before the tiles existed, and forever for a book with no cover art. open() is the same read path without the narration and costs no extra SD lookup. Measured on an X4, 43 books: shelf stage 285-306 ms -> 244-264 ms once baked, and the first paint that bakes four tiles costs ~272 ms extra, once.
SettingInfo measures ~188 bytes (three enum vectors, four std::function slots, a children vector), so the settings list is a chunk of heap in its own right and it is built on the path whose peak makes onEnter decide to silent-restart. Three things, in descending order of how much they cost: getSettingsListBase() reserved 64 but adds 69 entries, so the vector grew to capacity 128. It is a function-local static that lives for the whole process, meaning ~59 unused slots -- about 13 KB -- sat resident forever on a device with ~380 KB. Reserve 72, with a shrink_to_fit drift guard so outgrowing it later costs one transient reallocation instead of silently stranding the difference again. getSettingsList() copy-constructed from the base list, which allocates capacity exactly equal to the source, then always appended two font-fallback rows that are not in the base list. The first append therefore reallocated the whole vector, briefly holding the old ~69-element buffer and a new ~138-element one. Reserve up front so it is a single allocation. Reserve all 18 submenu vectors in rebuildSettingsLists. An unreserved push loop of 7 rows allocates at 1, 2, 4 and 8 elements and frees the first three; ~20 of these are built back to back, and onEnter's pre-flight refuses on maxAlloc as well as free bytes, so the freed odd-sized blocks matter as much as the total. Add a static_assert that SettingInfo stays nothrow-move-constructible. std::vector copies rather than moves when growing if the move constructor can throw, and a copy here deep-copies the enum vectors and std::functions. std::function's move is only noexcept from C++20, so this is load-bearing -- without it a future member could silently turn every push_back into a heap spike. Measured on an X4: rebuild peak -19196 -> ~-16600 bytes, free at Settings entry 73452 -> ~77000, and maxAlloc at entry 31732 -> 51188, i.e. clearing the 30 KB pre-flight floor by 21 KB instead of 1.7 KB.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Continues the perf pass from
HANDOFF-PERF-REMAINING.md. All numbers beloware measured on the user's X4 (43 books, 35 sleep images) from
PERF/RPROFserial captures, before and after each change.What changed
Library walk reuse across silent restarts. A navigation "loading moment"
on this device is a deliberate reboot, and each one re-walked the SD card it
had walked seconds earlier. An RTC-backed marker now lets a clean silent
restart reuse the previous boot's walk. Cold boot, power-cycle and crash
restarts always re-walk, so a swapped card is never trusted.
The correctness hazard is a book added or removed before the restart without
the index hearing about it -- the walk is how new books are discovered.
Arming is latched off for the rest of the boot by
CrossPointWebServer::begin(), which covers WS upload, HTTP upload, the filemanager's delete/rename and every WebDAV write verb in one place. Notably
Calibre Connect starts the same server, never touched
LibraryIndex, andsilent-restarts on exit -- a per-endpoint audit would have had to stay
exhaustive forever.
Concurrent library walks (data race). Adding timing to the walk showed
two per cold boot with overlapping intervals.
Activity::render()runs onthe render task,
Activity::loop()on the main task holding noRenderLock,and HomeActivity reaches the index from both.
ensureWalked()'s guard was acheck-then-set spanning the whole ~1.2 s walk, so both tasks passed it.
The wasted second was the lesser problem:
rescan()buildsstring_viewsinto the live path pool and then swaps all three pools wholesale, so a
concurrent rescan or
releaseMemory()leaves the other task's viewsdangling. Plausibly behind some of the unexplained reboots around Home.
Shelf paint. Two changes: probe the cached thumbnail first in
loadShelfCovers(an already-cached cell was paying four SD lookups toanswer a one-lookup question, including one hidden inside the
Epubconstructor), and cache pre-baked cover tiles via the existing
CoverTilesmechanism under a new role. The tile matters because a
.tileread needs nodecode and no image-cache budget -- the cold-boot shelf was slow because the
library walk fragments the heap just before it paints, so the bitmap cache
refused and every cell fell back to a streamed decode.
Settings heap peak. The base settings list reserved 64 but adds 69
entries, so a process-lifetime static sat at capacity 128 -- ~13 KB resident
forever on a ~380 KB device.
getSettingsList()also copy-constructed atexact capacity then always appended two rows, forcing a full reallocation.
Plus reserves on all 18 submenu vectors.
Measured
loadShelfCoversrebuildSettingsListspeakThat last row is the one that removes restarts rather than making them
faster: the pre-flight floor is 30 KB, so it went from clearing by 1.7 KB to
clearing by 21 KB.
Still open
to land near 250 ms against the 1563 ms measured pre-tiles, but unverified.
boot->readyon a cold boot can include ~3.6 s of blocking NTP(
runBootTimeNtpSyncOverBootScreen) when the clock is unset. Pre-existing,logs nothing, unrelated to this branch.
cps:serializereboot on Settings exit andgetSpineCumulativeSize index 8507in RecentBooksGrid both stillreproduce. Pre-existing, listed in the handoff.
simulatorenv does not build at HEAD (CollectionsStore.cppcallsStorage.writeFileWithBackupandString::reserve, absent from thesimulator's HAL shim). Untouched here;
tiny-bitterbuilds clean.Item D (refresh-mode audit) remains on hold per the handoff.
Build:
pio run -e tiny-bitter, 5,390,448 bytes, 1136 KB under the 6,553,600cap.