From 2635593890499e24994c7dd4c91c02662312175f Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Fri, 7 May 2021 03:59:27 +0200 Subject: [PATCH] Cache: Some more tweaks after #7624 * Allow doing away with CacheItem Now that we have working FFI finalizers on BBs, it's mostly useless overhead. We only keep it for DocCache, because it's slightly larger, and memory pressure might put us in a do or die situation where waiting for the GC might mean an OOM kill. * Expose's LRU slot-only mode And use it for CatalogCache, which doesn't care about storage space * Make GlyphCache slots only (storage space is insignificant here, it was always going to be evicted by running out of slots). * More informative warning when we chop the cache in half --- frontend/cache.lua | 61 +++++++++++++++++++++------ frontend/cacheitem.lua | 4 +- frontend/document/credocument.lua | 5 +-- frontend/document/doccache.lua | 2 + frontend/ui/rendertext.lua | 37 +++------------- frontend/ui/widget/imagewidget.lua | 23 ++++------ plugins/opds.koplugin/opdsbrowser.lua | 18 +++----- 7 files changed, 72 insertions(+), 78 deletions(-) diff --git a/frontend/cache.lua b/frontend/cache.lua index ddbbf316f..76c053992 100644 --- a/frontend/cache.lua +++ b/frontend/cache.lua @@ -14,10 +14,16 @@ end local Cache = { -- Cache configuration: - -- Max storage space, in bytes - size = 8 * 1024 * 1024, - -- Average item size is used to compute the amount of slots in the LRU - avg_itemsize = 8196, + -- Max storage space, in bytes... + size = nil, + -- ...Average item size, used to compute the amount of slots in the LRU. + avg_itemsize = nil, + -- Or, simply set the number of slots, with no storage space limitation. + -- c.f., GlyphCache, CatalogCache + slots = nil, + -- Should LRU call the object's onFree method on eviction? Implies using CacheItem instead of plain tables/objects. + -- c.f., DocCache + enable_eviction_cb = false, -- Generally, only DocCache uses this disk_cache = false, cache_path = nil, @@ -32,9 +38,14 @@ function Cache:new(o) end function Cache:init() - -- Compute the amount of slots in the LRU based on the max size & the average item size - self.slots = math.floor(self.size / self.avg_itemsize) - self.cache = lru.new(self.slots, self.size) + if self.slots then + -- Caller doesn't care about storage space, just slot count + self.cache = lru.new(self.slots, nil, self.enable_eviction_cb) + else + -- Compute the amount of slots in the LRU based on the max size & the average item size + self.slots = math.floor(self.size / self.avg_itemsize) + self.cache = lru.new(self.slots, self.size, self.enable_eviction_cb) + end if self.disk_cache then self.cached = self:_getDiskCache() @@ -42,6 +53,15 @@ function Cache:init() -- No need to go through our own check or even get methods if there's no disk cache, hit lru directly self.check = self.cache.get end + + if not self.enable_eviction_cb or not self.size then + -- We won't be using CacheItem here, so we can pass the size manually if necessary. + -- e.g., insert's signature is now (key, value, [size]), instead of relying on CacheItem's size field. + self.insert = self.cache.set + + -- With debug info (c.f., below) + --self.insert = self.set + end end --[[ @@ -157,14 +177,25 @@ function Cache:insert(key, object) self.cache:set(key, object, object.size) -- Accounting debugging - --[[ + --self:_insertion_stats(key, object.size) +end + +--[[ +function Cache:set(key, object, size) + self.cache:set(key, object, size) + + -- Accounting debugging + self:_insertion_stats(key, size) +end + +function Cache:_insertion_stats(key, size) print(string.format("Cache %s (%d/%d) [%.2f/%.2f @ ~%db] inserted %db key: %s", self, - self.cache:used_slots(), self.slots, self.cache:used_size() / 1024 / 1024, - self.size / 1024 / 1024, self.cache:used_size() / self.cache:used_slots(), - object.size, key)) - --]] + self.cache:used_slots(), self.slots, + self.cache:used_size() / 1024 / 1024, (self.size or 0) / 1024 / 1024, self.cache:used_size() / self.cache:used_slots(), + size or 0, key)) end +--]] --[[ -- check for cache item by key @@ -275,8 +306,10 @@ function Cache:memoryPressureCheck() end -- If less that 20% of the total RAM is free, drop half the Cache... - if memfree / memtotal < 0.20 then - logger.warn("Running low on memory, evicting half of the cache...") + local free_fraction = memfree / memtotal + if free_fraction < 0.20 then + logger.warn(string.format("Running low on memory (~%d%%, ~%.2f/%d MiB), evicting half of the cache...", + free_fraction * 100, memfree / 1024 / 1024, memtotal / 1024 / 1024)) self.cache:chop() -- And finish by forcing a GC sweep now... diff --git a/frontend/cacheitem.lua b/frontend/cacheitem.lua index 17bbf412f..7f0927736 100644 --- a/frontend/cacheitem.lua +++ b/frontend/cacheitem.lua @@ -17,8 +17,8 @@ function CacheItem:new(o) end -- Called on eviction. --- We generally use it to free C/FFI ressources *immediately* (as opposed to relying on our Userdata/FFI finalizers to do it "later" on GC). --- c.f., TileCacheItem, GlyphCacheItem & ImageCacheItem +-- We generally use it to free C/FFI resources *immediately* (as opposed to relying on our Userdata/FFI finalizers to do it "later" on GC). +-- c.f., TileCacheItem function CacheItem:onFree() end diff --git a/frontend/document/credocument.lua b/frontend/document/credocument.lua index 976f8be4a..290ec7caa 100644 --- a/frontend/document/credocument.lua +++ b/frontend/document/credocument.lua @@ -1379,7 +1379,7 @@ function CreDocument:setupCallCache() if self._tag_list_call_cache then self._tag_list_call_cache:clear() else - self._tag_list_call_cache = lru.new(10) + self._tag_list_call_cache = lru.new(10, nil, false) end -- i.e., the only thing that follows any sort of LRU eviction logic is the *list* of tag caches. -- Each individual cache itself is just a simple key, value store (i.e., a hash map). @@ -1410,8 +1410,7 @@ function CreDocument:setupCallCache() self._tag_call_cache = self._tag_list_call_cache:get(tag) if not self._tag_call_cache then -- Otherwise, create it and insert it in the list cache, evicting the LRU tag cache if necessary. - -- NOTE: We need CacheItem for its NOP onFree eviction callback. - self._tag_call_cache = CacheItem:new{} + self._tag_call_cache = {} self._tag_list_call_cache:set(tag, self._tag_call_cache) end self._current_call_cache_tag = tag diff --git a/frontend/document/doccache.lua b/frontend/document/doccache.lua index 4734089fd..893b38672 100644 --- a/frontend/document/doccache.lua +++ b/frontend/document/doccache.lua @@ -17,6 +17,8 @@ local DocCache = Cache:new{ size = calcCacheMemSize(), -- Average item size is a screen's worth of bitmap, mixed with a few much smaller tables (pgdim, pglinks, etc.), hence the / 3 avg_itemsize = math.floor(CanvasContext:getWidth() * CanvasContext:getHeight() * (CanvasContext.is_color_rendering_enabled and 4 or 1) / 3), + -- Rely on CacheItem's eviction callback to free resources *immediately* on eviction. + enable_eviction_cb = true, disk_cache = true, cache_path = DataStorage:getDataDir() .. "/cache/", } diff --git a/frontend/ui/rendertext.lua b/frontend/ui/rendertext.lua index b2e7ad318..2b119bf83 100644 --- a/frontend/ui/rendertext.lua +++ b/frontend/ui/rendertext.lua @@ -5,7 +5,6 @@ Text rendering module. local bit = require("bit") local Font = require("ui/font") local Cache = require("cache") -local CacheItem = require("cacheitem") local Blitbuffer = require("ffi/blitbuffer") local Device = require("device") local logger = require("logger") @@ -24,18 +23,12 @@ end local RenderText = {} local GlyphCache = Cache:new{ - -- 1 MiB of glyph cache, with 1024 slots - size = 1 * 1024 * 1024, - avg_itemsize = 1024, + -- 1024 slots + slots = 1024, + -- Rely on our FFI finalizer to free the BBs on GC + enable_eviction_cb = false, } -local GlyphCacheItem = CacheItem:new{} - -function GlyphCacheItem:onFree() - logger.dbg("GlyphCacheItem: free blitbuffer", self.bb) - self.bb:free() -end - -- iterator over UTF8 encoded characters in a string local function utf8Chars(input_text) local function read_next_glyph(input, pos) @@ -117,16 +110,7 @@ function RenderText:getGlyph(face, charcode, bold) logger.warn("error rendering glyph (charcode=", charcode, ") for face", face) return end - glyph = GlyphCacheItem:new{ - bb = rendered_glyph.bb, - l = rendered_glyph.l, - t = rendered_glyph.t, - r = rendered_glyph.r, - ax = rendered_glyph.ax, - ay = rendered_glyph.ay, - } - glyph.size = tonumber(glyph.bb.stride) * glyph.bb.h + 320 - GlyphCache:insert(hash, glyph) + GlyphCache:insert(hash, rendered_glyph) return rendered_glyph end @@ -325,16 +309,7 @@ function RenderText:getGlyphByIndex(face, glyphindex, bold) logger.warn("error rendering glyph (glyphindex=", glyphindex, ") for face", face) return end - glyph = GlyphCacheItem:new{ - bb = rendered_glyph.bb, - l = rendered_glyph.l, - t = rendered_glyph.t, - r = rendered_glyph.r, - ax = rendered_glyph.ax, - ay = rendered_glyph.ay, - } - glyph.size = tonumber(glyph.bb.stride) * glyph.bb.h + 320 - GlyphCache:insert(hash, glyph) + GlyphCache:insert(hash, rendered_glyph) return rendered_glyph end diff --git a/frontend/ui/widget/imagewidget.lua b/frontend/ui/widget/imagewidget.lua index 49d19735c..4f096441f 100644 --- a/frontend/ui/widget/imagewidget.lua +++ b/frontend/ui/widget/imagewidget.lua @@ -22,7 +22,6 @@ Show image from memory example: local Blitbuffer = require("ffi/blitbuffer") local Cache = require("cache") -local CacheItem = require("cacheitem") local Geom = require("ui/geometry") local RenderImage = require("ui/renderimage") local Screen = require("device").screen @@ -45,15 +44,10 @@ local ImageCache = Cache:new{ -- hence the leeway. size = 8 * 1024 * 1024, avg_itemsize = 64 * 1024, + -- Rely on our FFI finalizer to free the BBs on GC + enable_eviction_cb = false, } -local ImageCacheItem = CacheItem:new{} - -function ImageCacheItem:onFree() - logger.dbg("ImageCacheItem: free blitbuffer", self.bb) - self.bb:free() -end - local ImageWidget = Widget:new{ -- Can be provided with a path to a file file = nil, @@ -150,12 +144,12 @@ function ImageWidget:_loadfile() hash = hash .. "|d" self.already_scaled_for_dpi = true -- so we don't do it again in _render() end - local cache = ImageCache:check(hash) - if cache then + local cached = ImageCache:check(hash) + if cached then -- hit cache - self._bb = cache.bb + self._bb = cached.bb self._bb_disposable = false -- don't touch or free a cached _bb - self._is_straight_alpha = cache.is_straight_alpha + self._is_straight_alpha = cached.is_straight_alpha else if itype == "svg" then local zoom @@ -222,12 +216,11 @@ function ImageWidget:_loadfile() self._bb_disposable = false -- don't touch or free a cached _bb -- cache this image logger.dbg("cache", hash) - cache = ImageCacheItem:new{ + cached = { bb = self._bb, is_straight_alpha = self._is_straight_alpha, } - cache.size = tonumber(cache.bb.stride) * cache.bb.h - ImageCache:insert(hash, cache) + ImageCache:insert(hash, cached, tonumber(cached.bb.stride) * cached.bb.h) end end else diff --git a/plugins/opds.koplugin/opdsbrowser.lua b/plugins/opds.koplugin/opdsbrowser.lua index 6abfe3a45..d8b0ac00f 100644 --- a/plugins/opds.koplugin/opdsbrowser.lua +++ b/plugins/opds.koplugin/opdsbrowser.lua @@ -2,7 +2,6 @@ local BD = require("ui/bidi") local ButtonDialog = require("ui/widget/buttondialog") local ButtonDialogTitle = require("ui/widget/buttondialogtitle") local Cache = require("cache") -local CacheItem = require("cacheitem") local ConfirmBox = require("ui/widget/confirmbox") local DocumentRegistry = require("document/documentregistry") local InfoMessage = require("ui/widget/infomessage") @@ -24,15 +23,10 @@ local util = require("util") local _ = require("gettext") local T = require("ffi/util").template -local CatalogCacheItem = CacheItem:new{ - size = 1024, -- fixed size for catalog items -} - -- cache catalog parsed from feed xml local CatalogCache = Cache:new{ - -- Make it 20 slots - size = 20 * CatalogCacheItem.size, - avg_itemsize = CatalogCacheItem.size, + -- Make it 20 slots, with no storage space constraints + slots = 20, } local OPDSBrowser = Menu:extend{ @@ -328,23 +322,21 @@ function OPDSBrowser:fetchFeed(item_url, username, password, method) end function OPDSBrowser:parseFeed(item_url, username, password) - local feed local feed_last_modified = self:fetchFeed(item_url, username, password, "HEAD") local hash = "opds|catalog|" .. item_url if feed_last_modified then hash = hash .. "|" .. feed_last_modified end - local cache = CatalogCache:check(hash) - if cache then + local feed = CatalogCache:check(hash) + if feed then logger.dbg("Cache hit for", hash) - feed = cache.feed else logger.dbg("Cache miss for", hash) feed = self:fetchFeed(item_url, username, password) if feed then logger.dbg("Caching", hash) - CatalogCache:insert(hash, CatalogCacheItem:new{ feed = feed }) + CatalogCache:insert(hash, feed) end end if feed then