From f805477091babdfd6783671dfbe7aea7a47e0980 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Tue, 7 Jul 2026 00:55:51 +0900 Subject: [PATCH] perf(koplugin): defer and cache Library group covers (#4954) (#4974) Opening the Library with a large grouped library was slow because each folder's 2x2 cover mosaic was recomposed from scratch on every paint (up to 4 MuPDF cover decodes plus scales per cell). On a 685-book library this dominated the synchronous open path (254ms of 300ms) and ran again on the post-sync refresh. Navigation felt fast only because drilling into a group shows single covers, not mosaics. Mirror cloud_covers' async pattern in group_covers: - Cache the composed master bb per group, keyed by a signature that flips when the child set or any child's cover availability changes; serve cheap copies on a hit. Cache a nil result too, so a group whose covers are not ready yet keeps its placeholder without recomposing on every refresh; a later cover download flips the signature and recomposes once. - Compose off the first-paint path: a miss enqueues a background job (one mosaic per UI tick) and returns nil so the cell paints its FakeCover placeholder immediately; finished mosaics coalesce into one refresh. - Free cached masters when the Library closes. Measured synchronous open path drops from 300ms to 151ms on a 685-book library; mosaic compositing moves off the blocking paint and fills in progressively. Adds open-path timing logs to librarywidget and localscanner for on-device diagnosis of future large-library reports. Co-authored-by: Claude Opus 4.8 (1M context) --- .../readest.koplugin/library/cloud_covers.lua | 9 + .../readest.koplugin/library/group_covers.lua | 166 ++++++++++++++++-- apps/readest.koplugin/library/libraryitem.lua | 3 + .../library/librarywidget.lua | 37 +++- .../readest.koplugin/library/localscanner.lua | 22 ++- .../spec/library/group_covers_spec.lua | 98 +++++++++++ 6 files changed, 317 insertions(+), 18 deletions(-) diff --git a/apps/readest.koplugin/library/cloud_covers.lua b/apps/readest.koplugin/library/cloud_covers.lua index 06ffd077..0e8d6a07 100644 --- a/apps/readest.koplugin/library/cloud_covers.lua +++ b/apps/readest.koplugin/library/cloud_covers.lua @@ -78,6 +78,15 @@ function M.load_cover_bb(hash) return bb end +-- Whether the on-disk .png cover cache exists. Cheap (a stat, no +-- decode) — group_covers uses it to build its mosaic cache signature so a +-- late-arriving cover flips the signature and forces one recompose. +function M.cover_exists(hash) + if not hash or hash == "" then return false end + local lfs = require("libs/libkoreader-lfs") + return lfs.attributes(cover_path_for(hash), "mode") == "file" +end + -- " ''" formatted log tag — searchable by either id. local function tag_for(hash) local meta = _meta[hash] or {} diff --git a/apps/readest.koplugin/library/group_covers.lua b/apps/readest.koplugin/library/group_covers.lua index 34c46b83..8f593257 100644 --- a/apps/readest.koplugin/library/group_covers.lua +++ b/apps/readest.koplugin/library/group_covers.lua @@ -1,15 +1,19 @@ -- group_covers.lua -- macOS-style folder previews: a 2x2 mosaic of the first N child book -- covers, served as a synthetic readest-group:// URI through the --- patched BookInfoManager. Composites are recomposed in memory on every --- paint — no on-disk cache. Earlier versions cached PNGs under --- <settings>/readest_group_covers/, content-fingerprinted by child --- hashes, but any partial composite written while children's covers --- were still downloading would freeze: the fingerprint stayed the same --- once all four arrived, so the partial PNG kept serving forever. --- Recomposing each paint is cheap (4 small covers + scale + blit) and --- side-steps that cache-coherency surface entirely. +-- patched BookInfoManager. +-- +-- Composites are cached in memory (per group) and composed off the paint +-- path — see the "Mosaic cache + background compositing" section below. +-- We deliberately keep NO on-disk cache: an earlier version wrote PNGs +-- under <settings>/readest_group_covers/ fingerprinted by child hashes, +-- but a partial composite written while children's covers were still +-- downloading kept serving forever, because the hash-only fingerprint +-- didn't change once the late covers arrived. The in-memory cache keys on +-- child-cover *availability* too, so a late cover flips the key and forces +-- exactly one recompose. +local logger = require("logger") local cloud_covers = require("library.cloud_covers") local M = {} @@ -151,10 +155,120 @@ function M.cells_for(shape) return layout.cols * layout.rows end --- High-level: query the store, compose a fresh mosaic, return (bb, --- books). books is the resolved list (so callers can reuse it without a --- second query). bb is freshly composed every call — see the module --- header for why we deliberately don't cache. +-- --------------------------------------------------------------------------- +-- Mosaic cache + background compositing (issue #4954) +-- --------------------------------------------------------------------------- +-- Recomposing a folder mosaic on every paint (up to 4 MuPDF cover decodes + +-- scales per cell) dominated the Library's open cost on large libraries. +-- Two fixes, mirroring cloud_covers' async download pattern: +-- * cache the composed master bb per group, keyed by a signature that +-- flips when the child set OR any child's cover availability changes; +-- serve cheap copies on a hit. +-- * compose off the first-paint path: a miss enqueues a background job +-- (one per UI tick) and returns nil so the cell paints its FakeCover +-- placeholder immediately; finished mosaics coalesce into one refresh. +local _mosaic_cache = {} -- identity → { key = signature, bb = master } +local _compose_queue = {} -- FIFO of pending compose jobs +local _compose_pending = {} -- identity → true while queued/in-flight +local _refresh_pending = false +local _pump_scheduled = false + +-- Whether a child's cover can be produced without a network fetch: a local +-- book whose file is on disk, or a cloud book whose <hash>.png is cached. +-- Cheap (a stat, no image decode) — only used to build the cache signature. +function M.child_cover_available(book) + if not book then return false end + if book.local_present == 1 and book.file_path then + local lfs = require("libs/libkoreader-lfs") + if lfs.attributes(book.file_path, "mode") == "file" then return true end + end + if book.hash and book.hash ~= "" and cloud_covers.cover_exists(book.hash) then + return true + end + return false +end + +-- Signature that changes exactly when a mosaic's inputs change: the ordered +-- child hashes plus a per-child cover-availability bit. A keyed lookup on +-- this recomposes on child-set changes and when a missing cover arrives, and +-- hits otherwise. "\0" joins fields that can't contain it. +function M.mosaic_cache_key(group_by, value, shape, books) + local n = M.cells_for(shape) + local parts = {} + for i = 1, math.min(n, #books) do + local b = books[i] + parts[i] = (b.hash or "?") .. (M.child_cover_available(b) and "+" or "-") + end + return table.concat({ group_by, value, shape, table.concat(parts, ",") }, "\0") +end + +-- ImageWidget frees whatever bb it's handed, so the cached master can't be +-- shared directly — every serve returns a disposable copy. +local function copy_bb(src) + local Blitbuffer = require("ffi/blitbuffer") + local dst = Blitbuffer.new(src:getWidth(), src:getHeight(), src:getType()) + dst:blitFrom(src, 0, 0, 0, 0, src:getWidth(), src:getHeight()) + return dst +end + +local function store_master(identity, key, bb) + local prev = _mosaic_cache[identity] + if prev and prev.bb and prev.bb ~= bb then prev.bb:free() end + _mosaic_cache[identity] = { key = key, bb = bb } +end + +-- Coalesce mosaic-completion repaints: many mosaics landing across ticks +-- still trigger one Library refresh, not a flicker storm. +local function schedule_refresh() + if _refresh_pending then return end + _refresh_pending = true + local UIManager = require("ui/uimanager") + UIManager:nextTick(function() + _refresh_pending = false + local ok, LibraryWidget = pcall(require, "library.librarywidget") + if ok and LibraryWidget._menu then LibraryWidget.refresh() end + end) +end + +-- Background compose pump: one mosaic per UI tick so a page of folders fills +-- in progressively instead of freezing the paint. _pump_scheduled coalesces +-- the nextTick so N misses in one paint don't stack N pumps. +local process_compose_queue -- forward declaration for schedule_pump + +local function schedule_pump() + if _pump_scheduled then return end + _pump_scheduled = true + local UIManager = require("ui/uimanager") + UIManager:nextTick(process_compose_queue) +end + +process_compose_queue = function() + _pump_scheduled = false + local job = table.remove(_compose_queue, 1) + if not job then return end + _compose_pending[job.identity] = nil + -- Cache the result under this availability signature even when compose + -- returns nil (no child cover ready → the cell keeps its FakeCover + -- placeholder). Without caching the nil, a coverless group would miss + -- and recompose on every refresh forever; a later cover download flips + -- the signature (the key), which misses and recomposes exactly once. + store_master(job.identity, job.key, compose(job.books, job.shape, + job.orig_getBookInfo, job.BIM)) + schedule_refresh() + if #_compose_queue > 0 then schedule_pump() end +end + +local function enqueue_compose(job) + if _compose_pending[job.identity] then return end + _compose_pending[job.identity] = true + _compose_queue[#_compose_queue + 1] = job + schedule_pump() +end + +-- High-level: resolve the group's first-N children, then either serve the +-- cached master (as a copy) or enqueue a background compose and return nil +-- so the cell shows its FakeCover placeholder until the mosaic lands. Second +-- return is the resolved child list, so callers reuse it without re-querying. function M.serve_or_compose(group_by, value, shape, store, settings, orig_getBookInfo, BIM) if not store then return nil, {} end @@ -163,7 +277,33 @@ function M.serve_or_compose(group_by, value, shape, sort_by = settings and settings.library_sort_by, sort_asc = settings and settings.library_sort_ascending == true, }) - return compose(books, shape, orig_getBookInfo, BIM), books + local identity = table.concat({ group_by, value, shape }, "\0") + local key = M.mosaic_cache_key(group_by, value, shape, books) + local entry = _mosaic_cache[identity] + if entry and entry.key == key then + -- Already composed for this exact signature: serve a copy of the + -- master, or nil (placeholder) if no cover was available — but do + -- NOT re-enqueue, else a coverless group recomposes every refresh. + return entry.bb and copy_bb(entry.bb) or nil, books + end + enqueue_compose({ + identity = identity, key = key, books = books, shape = shape, + orig_getBookInfo = orig_getBookInfo, BIM = BIM, + }) + logger.dbg("ReadestLibrary mosaic miss, composing in background: " .. identity) + return nil, books +end + +-- Free cached masters when the Library closes so a big grid doesn't pin +-- ~0.7MB per folder for the app's lifetime; reopen recomposes in the +-- background (non-blocking). +function M.clear_cache() + for _identity, entry in pairs(_mosaic_cache) do + if entry.bb then entry.bb:free() end + end + _mosaic_cache = {} + _compose_queue = {} + _compose_pending = {} end return M diff --git a/apps/readest.koplugin/library/libraryitem.lua b/apps/readest.koplugin/library/libraryitem.lua index 89d44092..4ab06f57 100644 --- a/apps/readest.koplugin/library/libraryitem.lua +++ b/apps/readest.koplugin/library/libraryitem.lua @@ -47,6 +47,9 @@ end function M.set_visible_hashes(menu) if not menu then cloud_covers.set_visible_hashes(nil) + -- Library closing: drop cached group-mosaic masters so they don't + -- pin memory for the app's lifetime (issue #4954). + group_covers.clear_cache() return end diff --git a/apps/readest.koplugin/library/librarywidget.lua b/apps/readest.koplugin/library/librarywidget.lua index ef9d85a7..5a1763b6 100644 --- a/apps/readest.koplugin/library/librarywidget.lua +++ b/apps/readest.koplugin/library/librarywidget.lua @@ -17,10 +17,19 @@ local NetworkMgr = require("ui/network/manager") local TitleBar = require("ui/widget/titlebar") local Trapper = require("ui/trapper") local UIManager = require("ui/uimanager") +local time = require("ui/time") local logger = require("logger") local _ = require("readest_i18n") local T = require("ffi/util").template +-- Milliseconds elapsed since a ui/time snapshot, floored to an int for logs. +-- Temporary open-path instrumentation for issue #4954 (large-library load +-- speed): times each synchronous stage so a reporter's crash.log pinpoints +-- which one dominates before we commit to a fix. +local function elapsed_ms(since) + return math.floor(time.to_ms(time.since(since))) +end + local LibraryStore = require("library.librarystore") local libraryitem = require("library.libraryitem") local librarypaint = require("library.librarypaint") @@ -498,9 +507,14 @@ local function runCloudSync(opts, store) logger.info("ReadestLibrary runCloudSync: mode=" .. mode .. " auto_sync=" .. tostring(opts.settings.auto_sync)) + -- Deferred (post-paint) but still on the UI loop, so a slow network sync + -- can freeze the Library after it appears — time it so the reporter's log + -- distinguishes this from the synchronous open-path cost (issue #4954). + local t_sync = time.now() local function done(success, msg, status) logger.info("ReadestLibrary runCloudSync[" .. mode .. "] done: success=" - .. tostring(success) .. " msg=" .. tostring(msg) .. " status=" .. tostring(status)) + .. tostring(success) .. " msg=" .. tostring(msg) .. " status=" .. tostring(status) + .. " elapsed=" .. elapsed_ms(t_sync) .. "ms") M.refresh() end @@ -546,9 +560,15 @@ local function runOpenSync(opts, store, menu) -- has been uploaded to Readest cloud yet) would never appear in -- the menu, since the initial item_table was built from the -- pre-scan store snapshot. + local t_scan = time.now() local ok, err = pcall(localscanner.lightScan, { store = store }) + logger.info(string.format("ReadestLibrary runOpenSync: lightScan %dms", + elapsed_ms(t_scan))) if not ok then logger.warn("ReadestLibrary lightScan failed:", err) end + local t_refresh = time.now() M.refresh() + logger.info(string.format("ReadestLibrary runOpenSync: post-scan refresh %dms", + elapsed_ms(t_refresh))) -- 2. Cloud sync — deferred so the menu paints first. -- willRerunWhenOnline + the inline call moved into the @@ -579,6 +599,7 @@ function M.open(opts, internal) return end + local t_open = time.now() M._opts = opts -- Each fresh open starts at the root shelf; group drill-in state is -- per-session, never persisted to disk. M.reopen passes keep_state @@ -670,13 +691,19 @@ function M.open(opts, internal) local portrait = Screen:getWidth() <= Screen:getHeight() local items_per_page = portrait and (nb_cols_p * nb_rows_p) or (nb_cols_l * nb_rows_l) + local t_build = time.now() + local initial_items = build_item_table(store, opts.settings, M._search) + logger.info(string.format( + "ReadestLibrary open: initial build_item_table %dms (%d items)", + elapsed_ms(t_build), #initial_items)) + menu = Menu:new{ name = "readest_library", is_borderless = true, is_popout = false, covers_fullscreen = true, custom_title_bar = title_bar, - item_table = build_item_table(store, opts.settings, M._search), + item_table = initial_items, width = Screen:getWidth(), height = Screen:getHeight(), items_per_page = items_per_page, @@ -752,6 +779,12 @@ function M.open(opts, internal) UIManager:show(menu) runOpenSync(opts, store, menu) + -- Everything above ran synchronously inside the tap handler, so the UI + -- cannot repaint until M.open returns — this is the "blocks first paint" + -- window the reporter perceives as slow load (issue #4954). + logger.info(string.format( + "ReadestLibrary open: total synchronous open path %dms (blocks first paint)", + elapsed_ms(t_open))) end -- --------------------------------------------------------------------------- diff --git a/apps/readest.koplugin/library/localscanner.lua b/apps/readest.koplugin/library/localscanner.lua index 528e7333..13714a86 100644 --- a/apps/readest.koplugin/library/localscanner.lua +++ b/apps/readest.koplugin/library/localscanner.lua @@ -110,11 +110,17 @@ function M.lightScan(opts) local lfs = require("libs/libkoreader-lfs") local DocSettings = require("docsettings") local ReadHistory = require("readhistory") + -- Open-path timing for issue #4954: lazily required here so the module + -- top stays free of live-KOReader deps and the pure-helper specs can + -- still `require("library.localscanner")`. + local time = require("ui/time") + local function ms(since) return math.floor(time.to_ms(time.since(since))) end local store = opts.store if not store then return 0, 0 end -- Step 1: sweep stale file_paths to local_present=0 + local t_step1 = time.now() local stale = 0 local rows = store:listBooks({}) for _, row in ipairs(rows) do @@ -129,18 +135,24 @@ function M.lightScan(opts) end end end + local step1_ms = ms(t_step1) -- Step 2: opportunistic upsert from ReadHistory. -- Per-iteration pcall so a single bad sidecar (corrupt Lua, file -- vanished mid-scan, DocSettings throwing on open) doesn't kill the -- whole loop and leave us with a partially-indexed library. + local t_step2 = time.now() local added, skipped = 0, 0 + local hist_count, ds_count = 0, 0 for _, item in ipairs(ReadHistory.hist or {}) do + hist_count = hist_count + 1 local file = item.file if file and lfs.attributes(file, "mode") == "file" then local ok, err = pcall(function() -- DocSettings reads the sidecar for us; we get the same - -- hash the sidecar walk would produce. + -- hash the sidecar walk would produce. This open reads + + -- evaluates the sidecar Lua file from flash on every pass. + ds_count = ds_count + 1 local doc_settings = DocSettings:open(file) local hash = doc_settings:readSetting("partial_md5_checksum") if not hash or hash == "" then @@ -169,8 +181,12 @@ function M.lightScan(opts) end end - logger.info("ReadestLibrary lightScan: stale=" .. stale - .. " added/refreshed=" .. added .. " skipped=" .. skipped) + local step2_ms = ms(t_step2) + logger.info(string.format( + "ReadestLibrary lightScan: total=%dms | step1_sweep=%dms rows=%d stale=%d" + .. " | step2_history=%dms entries=%d docsettings_opens=%d added=%d skipped=%d", + step1_ms + step2_ms, step1_ms, #rows, stale, + step2_ms, hist_count, ds_count, added, skipped)) return stale, added end diff --git a/apps/readest.koplugin/spec/library/group_covers_spec.lua b/apps/readest.koplugin/spec/library/group_covers_spec.lua index 88e794d4..b9380e44 100644 --- a/apps/readest.koplugin/spec/library/group_covers_spec.lua +++ b/apps/readest.koplugin/spec/library/group_covers_spec.lua @@ -151,6 +151,104 @@ describe("library.group_covers", function() assert.are.equal(0, #trigger_calls) end) end) + + -- ===================================================================== + -- cover availability + mosaic cache key (issue #4954) + -- ===================================================================== + -- Group mosaics are cached and served as copies instead of recomposed + -- on every paint. The cache key must change exactly when a mosaic's + -- inputs change: the child set OR a previously-missing child cover + -- becoming available. The availability bit is what fixes the historical + -- "partial composite served forever" bug the old on-disk cache had. + describe("cover availability + mosaic cache key", function() + local real_lfs = require("lfs") + + local function write_file(path) + local f = assert(io.open(path, "w")) + f:write("x") + f:close() + end + + local function write_cover(hash) + local dir = cloud_covers.covers_dir() + real_lfs.mkdir(dir) + write_file(dir .. "/" .. hash .. ".png") + end + + describe("cloud_covers.cover_exists", function() + it("is false when no <hash>.png is on disk", function() + assert.is_false(cloud_covers.cover_exists("missing1")) + end) + + it("is true once the <hash>.png file exists", function() + write_cover("present1") + assert.is_true(cloud_covers.cover_exists("present1")) + end) + end) + + describe("group_covers.child_cover_available", function() + it("is true for a cloud child whose cover .png is cached", function() + write_cover("cloudcov") + assert.is_true(group_covers.child_cover_available( + { hash = "cloudcov", cloud_present = 1, local_present = 0 })) + end) + + it("is false for a cloud child whose cover isn't downloaded yet", function() + assert.is_false(group_covers.child_cover_available( + { hash = "nocover1", cloud_present = 1, local_present = 0 })) + end) + + it("is true for a local child whose file is on disk", function() + local path = require("datastorage"):getSettingsDir() .. "/localbook.epub" + write_file(path) + assert.is_true(group_covers.child_cover_available( + { hash = "localbk1", local_present = 1, file_path = path })) + end) + + it("is false when neither a local file nor a cached cover exists", function() + assert.is_false(group_covers.child_cover_available( + { hash = "ghost001", local_present = 1, file_path = "/no/such/file.epub" })) + end) + end) + + describe("group_covers.mosaic_cache_key", function() + local books = { + { hash = "aaa" }, { hash = "bbb" }, { hash = "ccc" }, { hash = "ddd" }, + } + + it("is stable across calls with identical inputs (enables cache hits)", function() + assert.are.equal( + group_covers.mosaic_cache_key("group_name", "Fantasy", "grid", books), + group_covers.mosaic_cache_key("group_name", "Fantasy", "grid", books)) + end) + + it("changes when the child set changes", function() + local other = { + { hash = "aaa" }, { hash = "zzz" }, { hash = "ccc" }, { hash = "ddd" }, + } + assert.are_not.equal( + group_covers.mosaic_cache_key("group_name", "Fantasy", "grid", books), + group_covers.mosaic_cache_key("group_name", "Fantasy", "grid", other)) + end) + + it("changes when a missing child cover becomes available (anti-regression)", function() + local one = { { hash = "latecov" } } + local before = group_covers.mosaic_cache_key("author", "Asimov", "grid", one) + write_cover("latecov") + local after = group_covers.mosaic_cache_key("author", "Asimov", "grid", one) + assert.are_not.equal(before, after) + end) + + it("distinguishes different groups and shapes", function() + assert.are_not.equal( + group_covers.mosaic_cache_key("group_name", "A", "grid", books), + group_covers.mosaic_cache_key("group_name", "A", "list", books)) + assert.are_not.equal( + group_covers.mosaic_cache_key("group_name", "A", "grid", books), + group_covers.mosaic_cache_key("group_name", "B", "grid", books)) + end) + end) + end) end) -- =====================================================================