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

@@ -14,10 +14,16 @@ end
 
 local Cache = {
     -- Cache configuration:
-    -- Max storage space, in bytes
+    -- Max storage space, in bytes...
-    size = 8 * 1024 * 1024,
+    size = nil,
-    -- Average item size is used to compute the amount of slots in the LRU
+    -- ...Average item size, used to compute the amount of slots in the LRU.
-    avg_itemsize = 8196,
+    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
+    if self.slots then
-    self.slots = math.floor(self.size / self.avg_itemsize)
+        -- Caller doesn't care about storage space, just slot count
-    self.cache = lru.new(self.slots, self.size)
+        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.cache:used_slots(), self.slots,
-                        self.size / 1024 / 1024, self.cache:used_size() / self.cache:used_slots(),
+                        self.cache:used_size() / 1024 / 1024, (self.size or 0) / 1024 / 1024, self.cache:used_size() / self.cache:used_slots(),
-                        object.size, key))
+                        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
+    local free_fraction = memfree / memtotal
-        logger.warn("Running low on memory, evicting half of the cache...")
+    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...

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).
+-- 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, GlyphCacheItem & ImageCacheItem
+-- c.f., TileCacheItem
 function CacheItem:onFree()
 end
 

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 = {}
-            self._tag_call_cache = CacheItem:new{}
             self._tag_list_call_cache:set(tag, self._tag_call_cache)
         end
         self._current_call_cache_tag = tag

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/",
 }

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
+    -- 1024 slots
-    size = 1 * 1024 * 1024,
+    slots = 1024,
-    avg_itemsize = 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{
+    GlyphCache:insert(hash, rendered_glyph)
-        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)
     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{
+    GlyphCache:insert(hash, rendered_glyph)
-        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)
     return rendered_glyph
 end
 

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)
+        local cached = ImageCache:check(hash)
-        if cache then
+        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, cached, tonumber(cached.bb.stride) * cached.bb.h)
-                ImageCache:insert(hash, cache)
             end
         end
     else

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
+    -- Make it 20 slots, with no storage space constraints
-    size = 20 * CatalogCacheItem.size,
+    slots = 20,
-    avg_itemsize = CatalogCacheItem.size,
 }
 
 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)
+    local feed = CatalogCache:check(hash)
-    if cache then
+    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