refactor(shell): replace function registry with LRU cache

Author: ibhagwan

.luarc.jsonc

@@ -3,7 +3,7 @@
   "runtime.version": "LuaJIT",
   "diagnostics": {
     "enable": true,
-    "globals": ["vim"],
+    "globals": ["vim", "describe", "it" ],
     "neededFileStatus": {
       "codestyle-check": "Any"
     },

lua/fzf-lua/core.lua

@@ -155,8 +155,7 @@ M.fzf_exec = function(contents, opts)
   -- the API accepts both tables and functions which we "stringify"
   -- We also send string commands as stringify is also responsible
   -- for multiprocess wrapping of shell commands with processing
-  shell.clear_protected()
-  contents = contents and shell.stringify(contents, opts, nil, true) or nil
+  contents = contents and shell.stringify(contents, opts, nil) or nil
   assert(contents == nil or type(contents) == "string", "contents must be of type string")
   return M.fzf_wrap(contents, opts, true)
 end
@@ -175,7 +174,6 @@ M.fzf_live = function(contents, opts)
   -- utilizes fzf's 'change:reload' event or skim's "interactive" mode
   -- convert "reload" actions to fzf's `reload` binds
   -- convert "exec_silent" actions to fzf's `execute-silent` binds
-  shell.clear_protected()
   if type(contents) == "string" then
     -- Signal to stringify_mt we are relocating <query>
     -- Signal to preprocess we are looking to replace {argvz}
@@ -185,7 +183,7 @@ M.fzf_live = function(contents, opts)
       contents = ("%s %s"):format(contents, M.fzf_query_placeholder)
     end
   end
-  opts.fn_reload = shell.stringify(contents, opts, nil, true)
+  opts.fn_reload = shell.stringify(contents, opts, nil)
   local fzf_field_index = M.fzf_field_index(opts)
   local cmd = M.expand_query(opts.fn_reload, fzf_field_index)
   contents, opts = M.setup_fzf_interactive_flags(cmd, fzf_field_index, opts)
@@ -1014,7 +1012,7 @@ local patch_shell_action = function(v, opts)
       items = (zero_matched and zero_selected) and {} or items
     end
     v.fn(items, opts)
-  end, opts, field_index, true)
+  end, opts, field_index)
 end
 
 -- converts actions defined with "reload=true" to use fzf's `reload` bind

lua/fzf-lua/previewer/builtin.lua

@@ -454,7 +454,7 @@ function Previewer.base:cmdline(_)
     -- save last entry even if we don't display
     self.last_entry = entry
     return ""
-  end, self.opts, "{} {q} {n}", false)
+  end, self.opts, "{} {q} {n}")
   return act
 end
 
@@ -481,7 +481,7 @@ function Previewer.base:zero(_)
         self.last_entry = nil
         vim.fn.delete(self._zero_lock, "d")
       end, self.delay)
-    end, self.opts, "", false))
+    end, self.opts, ""))
   return act
 end
 

lua/fzf-lua/previewer/fzf.lua

@@ -87,7 +87,7 @@ function Previewer.cmd:action(o)
   local act = shell.stringify_data(function(items, _, _)
     local entry = path.entry_to_file(items[1], self.opts)
     return entry.bufname or entry.path
-  end, self.opts, self.opts.field_index_expr or "{}", false)
+  end, self.opts, self.opts.field_index_expr or "{}")
   return act
 end
 

lua/fzf-lua/providers/colorschemes.lua

@@ -457,10 +457,9 @@ M.awesome_colorschemes = function(opts)
     end)()
   end
 
-  local prev_act_id
   if opts.live_preview then
     opts.fzf_opts["--preview-window"] = "nohidden:right:0"
-    opts.preview, prev_act_id = shell.stringify_data(function(sel)
+    opts.preview = shell.stringify_data(function(sel)
       if opts.live_preview and sel then
         local dbkey, idx = sel[1]:match("^(.-):(%d+):")
         if opts._adm:downloaded(dbkey) then
@@ -479,10 +478,6 @@ M.awesome_colorschemes = function(opts)
     end, opts, "{}")
   end
 
-  opts._fn_pre_fzf = function()
-    if prev_act_id then shell.set_protected(prev_act_id) end
-  end
-
   opts.winopts = opts.winopts or {}
   opts.winopts.on_close = function()
     -- reset color scheme if live_preview is enabled

lua/fzf-lua/shell.lua

@@ -5,43 +5,103 @@ local libuv = require "fzf-lua.libuv"
 local base64 = require "fzf-lua.lib.base64"
 local serpent = require "fzf-lua.lib.serpent"
 
-local M = {}
+---@class fzf-lua.lru
+---@field max_size integer
+---@field max_id integer
+---@field last_id integer
+---@field mru integer[]
+---@field store any[]
+---@field lookup table<string, integer>
+local LRU = {}
+
+function LRU:new(size)
+  local obj = {
+    max_size = size,
+    -- Arbitrary, should be big enough to assert
+    -- when accessing an evicted item
+    max_id = size * 20,
+    last_id = 0,
+    mru = {},
+    store = {},
+    lookup = {},
+  }
+  setmetatable(obj, self)
+  self.__index = self
+  return obj
+end
 
--- Circular buffer used to store registered function IDs
--- set max length to 10, ATM most actions used by a single
--- provider are 2 (`live_grep` with `multiprocess=false`)
--- and 4 (`git_status` with preview and 3 reload binds)
--- we can always increase if we need more
-local _MAX_LEN = 50
-local _index = 0
-local _registry = {}
-local _protected = {}
+---@param value any
+---@return integer, integer new element id and evicted (if evicted)
+function LRU:set(value)
+  local store_idx, evicted_id
+  local id = self.last_id + 1
+  if id > self.max_id then id = 1 end
+  if #self.store >= self.max_size then
+    -- Remove the last element in the MRU table
+    evicted_id = tostring(table.remove(self.mru))
+    store_idx = self.lookup[evicted_id]
+    self.lookup[evicted_id] = nil
+  else
+    store_idx = #self.store + 1
+  end
+  -- New id is always at the top of the MRU
+  table.insert(self.mru, 1, id)
+  self.store[store_idx] = value
+  self.lookup[tostring(id)] = store_idx
+  self.last_id = id
+  return id, tonumber(evicted_id)
+end
 
-function M.register_func(fn)
-  repeat
-    _index = _index % _MAX_LEN + 1
-  until not _protected[_index]
-  _registry[_index] = fn
-  return _index
+---@return integer
+function LRU:len()
+  return #self.store
 end
 
-function M.get_func(id)
-  return _registry[id]
+---@param id integer
+---@return any
+function LRU:get(id)
+  local store_idx = self.lookup[tostring(id)]
+  assert(store_idx, string.format("get nonexistent id %d", id))
+  self.mru = (function()
+    -- Remove all previous occurances of the id in the MRU
+    -- and insert the current id at the top
+    -- TODO: more efficient way?
+    local ret = {}
+    table.insert(ret, id)
+    for _, x in ipairs(self.mru) do
+      if x ~= id then
+        table.insert(ret, x)
+      end
+    end
+    return ret
+  end)()
+  return self.store[store_idx]
 end
 
-function M.set_protected(id)
-  _protected[id] = true
-  assert(_MAX_LEN > utils.tbl_count(_protected))
+-- Cache should be able to hold all function callbacks of a single picker
+-- max cache size of 50 should be more than enough, we don't want it to be
+-- too big as this will prevent clearing of referecnces to "opts" which
+-- prevents garabage collection from freeing the resources
+local function new_cache() return LRU:new(50) end
+local _cache = new_cache()
+
+local M = {}
+
+-- Export LRU for testing
+M.LRU = LRU
+
+function M.register_func(fn)
+  return (_cache:set(fn))
 end
 
-function M.clear_protected()
-  _protected = {}
+function M.get_func(id)
+  return _cache:get(id)
 end
 
-function M.clear_registry()
-  _index = 0
-  _registry = {}
-  _protected = {}
+-- NOTE: CI ONLY - DO NOT USE
+-- new cache is equal to cleared cache
+function M.clear_cache()
+  _cache = new_cache()
 end
 
 -- creates a new address to listen to messages from actions. This is important
@@ -246,7 +306,7 @@ end
 ---Fzf field index expression, e.g. "{+}" (selected), "{q}" (query)
 ---@param fzf_field_index string?
 ---@return string, integer?
-M.stringify = function(contents, opts, fzf_field_index, protect)
+M.stringify = function(contents, opts, fzf_field_index)
   assert(contents, "must supply contents")
 
   -- TODO: should we let this assert?
@@ -422,27 +482,19 @@ M.stringify = function(contents, opts, fzf_field_index, protect)
     end
   end, fzf_field_index or "", opts.debug)
 
-  -- "protect" a function ptr, cleared when opening a new picker in `core.fzf()`
-  -- protected functions include init (content) commands and acrions (reload
-  -- and execute-silent), previewer trigger commands are excluded (zero event
-  -- and preview callback)
-  if protect then
-    M.set_protected(id)
-  end
-
   return cmd, id
 end
 
-M.stringify_cmd = function(fn, opts, fzf_field_index, protect)
+M.stringify_cmd = function(fn, opts, fzf_field_index)
   assert(type(fn) == "function", "fn must be of type function")
   return M.stringify(fn, {
     __stringify_cmd = true,
     PidObject = utils.pid_object("__stringify_cmd_pid", opts),
     debug = opts.debug,
-  }, fzf_field_index, protect)
+  }, fzf_field_index)
 end
 
-M.stringify_data = function(fn, opts, fzf_field_index, protect)
+M.stringify_data = function(fn, opts, fzf_field_index)
   assert(type(fn) == "function", "fn must be of type function")
   return M.stringify(function(cb, _, ...)
     local ret = fn(...)
@@ -454,7 +506,7 @@ M.stringify_data = function(fn, opts, fzf_field_index, protect)
       cb(tostring(ret))
     end
     cb(nil)
-  end, { debug = opts.debug }, fzf_field_index, protect)
+  end, { debug = opts.debug }, fzf_field_index)
 end
 
 return M

tests/cache_spec.lua

@@ -0,0 +1,103 @@
+local helpers = require("fzf-lua.test.helpers")
+local assert = helpers.assert
+
+local fzf = require("fzf-lua")
+local LRU = fzf.shell.LRU
+
+describe("Testing cache module", function()
+  local cache
+  local cached_fun = function(id) return function() return "CACHED" .. tostring(id) end end
+
+  it("new", function()
+    local size = 50
+    cache = LRU:new(size)
+    assert.is.same(cache.max_size, size)
+  end)
+
+  it("first", function()
+    local id = cache:set(cached_fun(1))
+    assert.is.same(id, 1)
+    assert.is.same(cache:get(1)(), "CACHED1")
+  end)
+
+  it("half store bubble", function()
+    for _ = 1, 12 do cache:set(function() end) end
+    cache:set(cached_fun(14))
+    for _ = 1, 11 do cache:set(function() end) end
+    -- After inserting 25 elements the last element should be at the top
+    -- so basically the MRU is sorted in reverse order
+    assert.is.same(cache:len(), 25)
+    for i = 1, 25 do
+      assert.is.same(cache.mru[i], 25 - i + 1)
+    end
+    -- In reveerse order func14 is at [12] and fun1 is at [25]
+    assert.is.same(cache:len(), #cache.mru)
+    assert.is.same(cache.mru[12], 14)
+    assert.is.same(cache.mru[25], 1)
+    assert.is.same(cache:get(14)(), "CACHED14")
+    assert.is.same(cache:len(), #cache.mru)
+    -- After bubbling func14 should be moved to [1]
+    -- and rest of the elements should be shifted
+    assert.is.same(cache.mru[1], 14)
+    assert.is.same(cache.mru[11], 16)
+    assert.is.same(cache.mru[12], 15)
+    assert.is.same(cache.mru[13], 13)
+    assert.is.same(cache.mru[25], 1)
+  end)
+
+  it("full store", function()
+    -- Fill in the remaining 25 items
+    for _ = 1, 25 do cache:set(function() end) end
+    assert.is.same(cache:len(), 50)
+    -- New elements should take the first 25 slots
+    -- No element should be evicted at this point
+    for i = 1, 25 do
+      local _, evicted_id = assert.is.same(cache.mru[i], 50 - i + 1)
+      assert.is.same(evicted_id, nil)
+    end
+    -- Previous elements are shifted by 25
+    assert.is.same(cache.mru[1 + 25], 14)
+    assert.is.same(cache.mru[11 + 25], 16)
+    assert.is.same(cache.mru[12 + 25], 15)
+    assert.is.same(cache.mru[13 + 25], 13)
+    assert.is.same(cache.mru[25 + 25], 1)
+  end)
+
+  it("full store bubble", function()
+    -- Calling `:get()` bubbles the item in the MRU
+    assert.is.same(cache:get(14)(), "CACHED14")
+    assert.is.same(cache:len(), #cache.mru)
+    assert.is.same(cache.mru[1], 14)
+    assert.is.same(cache.mru[50], 1)
+    -- After bubbling func1, func14 shifts downward
+    assert.is.same(cache:get(1)(), "CACHED1")
+    assert.is.same(cache:len(), #cache.mru)
+    assert.is.same(cache.mru[1], 1)
+    assert.is.same(cache.mru[2], 14)
+    assert.is.same(cache.mru[50], 2)
+  end)
+
+  it("eviction", function()
+    -- Store a new function, should have an incremental id
+    -- func2 is evicted as it's at the bottom of the MRU
+    local id, evicted_id = cache:set(cached_fun(51))
+    assert.is.same(cache:len(), #cache.mru)
+    assert.is.same(id, 51)
+    assert.is.same(evicted_id, 2)
+    -- func51 gets the top spot at the MRU
+    assert.is.same(cache.mru[1], 51)
+    assert.is.same(cache.mru[2], 1)
+    assert.is.same(cache.mru[3], 14)
+    assert.is.same(cache.mru[50], 3)
+  end)
+
+  it("yet another bubble", function()
+    assert.is.same(cache:get(14)(), "CACHED14")
+    assert.is.same(cache:len(), #cache.mru)
+    assert.is.same(cache.mru[1], 14)
+    assert.is.same(cache.mru[2], 51)
+    assert.is.same(cache.mru[3], 1)
+    assert.is.same(cache.mru[50], 3)
+  end)
+
+end)

tests/screenshots/tests-api_spec.lua---api---fzf_exec---rg---1-+-args-{-1-}

@@ -2,7 +2,7 @@
 01|                                                                
 02|~                                                               
 03|~      ╭─────────────────────────────────────────────────╮      
-04|~      │>                                        120/120 │      
+04|~      │>                                        121/121 │      
 05|~      │──────────────────────────────────────────────── │      
 06|~      │▌ LICENSE                                       ││      
 07|~      │  Makefile                                      ││      

tests/screenshots/tests-api_spec.lua---api---fzf_exec---rg---1-+-args-{-false-}

@@ -2,7 +2,7 @@
 01|                                                                
 02|~                                                               
 03|~      ╭─────────────────────────────────────────────────╮      
-04|~      │>                                        121/121 │      
+04|~      │>                                        122/122 │      
 05|~      │──────────────────────────────────────────────── │      
 06|~      │▌ [DEBUG] [st] rg --files -g "!.git" --sort=path││      
 07|~      │  LICENSE                                       ││