From abced343d8a92eb928b234da5ef48e094bd2f533 Mon Sep 17 00:00:00 2001
From: Lars Mueller <appgurulars@gmx.de>
Date: Sat, 3 Jul 2021 19:35:02 +0200
Subject: [PATCH] Deal with Lua log file memory leak

---
 Readme.md       |  9 +++++++++
 persistence.lua | 34 ++++++++++++++++++++++++++--------
 test.lua        | 25 +++++++++++++++++--------
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/Readme.md b/Readme.md
index 7f86bb9..f42268b 100644
--- a/Readme.md
+++ b/Readme.md
@@ -27,6 +27,15 @@ logfile:init()
 assert(table.equals(logfile.root, {[{a = 1}] = {b = 2, c = 3}}))
 ```
 
+Both strings and tables are stored in a reference table. Unused strings won't be garbage collected as Lua doesn't allow marking them as weak references.
+This means that setting lots of temporary strings will waste memory until you call `:rewrite()` on the log file. An alternative is to set the third parameter, `reference_strings`, to `false` (default value is `true`):
+
+```lua
+persistence.lua_log_file.new(mod.get_resource"logfile.test.lua", {}, false)
+```
+
+This will prevent strings from being referenced, possibly bloating file size, but saving memory.
+
 ### Bluon
 
 Binary Lua object notation. **Experimental.** Handling of subnormal numbers (very small floats) may be broken.
diff --git a/persistence.lua b/persistence.lua
index b6a39eb..525e469 100644
--- a/persistence.lua
+++ b/persistence.lua
@@ -5,18 +5,31 @@ local assert, error, io, ipairs, loadfile, math, minetest, modlib, pairs, setfen
 local _ENV = {}
 setfenv(1, _ENV)
 
-lua_log_file = {}
+lua_log_file = {
+	-- default value
+	reference_strings = true
+}
 local files = {}
 local metatable = {__index = lua_log_file}
 
-function lua_log_file.new(file_path, root)
-	local self = setmetatable({file_path = assert(file_path), root = root}, metatable)
+function lua_log_file.new(file_path, root, reference_strings)
+	local self = setmetatable({
+		file_path = assert(file_path),
+		root = root,
+		reference_strings = reference_strings
+	}, metatable)
 	if minetest then
 		files[self] = true
 	end
 	return self
 end
 
+local function set_references(self, table)
+	-- Weak table keys to allow the collection of dead reference tables
+	-- TODO garbage collect strings in the references table
+	self.references = setmetatable(table, {__mode = "k"})
+end
+
 function lua_log_file:load()
 	-- Bytecode is blocked by the engine
 	local read = assert(loadfile(self.file_path))
@@ -27,7 +40,7 @@ function lua_log_file:load()
 	env.R = env.R or {{}}
 	self.reference_count = #env.R
 	self.root = env.R[1]
-	self.references = modlib.table.flip(env.R)
+	set_references(self, {})
 end
 
 function lua_log_file:open()
@@ -99,12 +112,13 @@ function lua_log_file:_dump(value, is_key)
 		self.references[value] = reference
 	end
 	if _type == "string" then
-		if is_key and value:len() <= key:len() and value:match"[%a_][%a%d_]*" then
+		local reference_strings = self.reference_strings
+		if is_key and ((not reference_strings) or value:len() <= key:len()) and value:match"[%a_][%a%d_]*" then
 			-- Short key
 			return value, true
 		end
 		formatted = ("%q"):format(value)
-		if formatted:len() <= key:len() then
+		if (not reference_strings) or formatted:len() <= key:len()  then
 			-- Short string
 			return formatted
 		end
@@ -133,10 +147,14 @@ function lua_log_file:_dump(value, is_key)
 end
 
 function lua_log_file:set(table, key, value)
-	table[key] = value
 	if not self.references[table] then
 		error"orphan table"
 	end
+	if table[key] == value then
+		-- No change
+		return
+	end
+	table[key] = value
 	table = self:_dump(table)
 	local key, short_key = self:_dump(key, true)
 	self:log(table .. (short_key and ("." .. key) or ("[" .. key .. "]")) .. "=" .. self:_dump(value))
@@ -147,7 +165,7 @@ function lua_log_file:set_root(key, value)
 end
 
 function lua_log_file:_write()
-	self.references = {}
+	set_references(self, {})
 	self.reference_count = 0
 	self:log"R={}"
 	self:_dump(self.root)
diff --git a/test.lua b/test.lua
index e230c8e..ffff5b5 100644
--- a/test.lua
+++ b/test.lua
@@ -224,14 +224,23 @@ test_from_string("#333", 0x333333FF)
 test_from_string("#694269", 0x694269FF)
 test_from_string("#11223344", 0x11223344)
 
-local logfile = persistence.lua_log_file.new(mod.get_resource"logfile.test.lua", {})
-logfile:init()
-logfile.root = {}
-logfile:rewrite()
-logfile:set_root({a = 1}, {b = 2, c = 3, d = _G.math.huge, e = -_G.math.huge})
-logfile:close()
-logfile:init()
-assert(table.equals(logfile.root, {[{a = 1}] = {b = 2, c = 3, d = _G.math.huge, e = -_G.math.huge}}))
+local function test_logfile(reference_strings)
+	local logfile = persistence.lua_log_file.new(mod.get_resource"logfile.test.lua", {}, reference_strings)
+	logfile:init()
+	logfile.root = {a_longer_string = "test"}
+	logfile:rewrite()
+	logfile:set_root({a = 1}, {b = 2, c = 3, d = _G.math.huge, e = -_G.math.huge})
+	logfile:close()
+	logfile:init()
+	assert(table.equals(logfile.root, {a_longer_string = "test", [{a = 1}] = {b = 2, c = 3, d = _G.math.huge, e = -_G.math.huge}}))
+	if not reference_strings then
+		for key in pairs(logfile.references) do
+			assert(type(key) ~= "string")
+		end
+	end
+end
+test_logfile(true)
+test_logfile(false)
 
 -- in-game tests & b3d testing
 local tests = {