From 624e2739579e0708fa162e67c3ce34b52865aa80 Mon Sep 17 00:00:00 2001 From: Dave Lucia Date: Thu, 11 Jun 2026 05:10:06 -0700 Subject: [PATCH] fix(stdlib): route package.loaded cache through Table.put so required modules are visible to pairs The package.loaded cache wrote the required module's key directly into loaded_table.data via Map.put, bypassing Table.put and its iteration-order bookkeeping. Key-access (package.loaded[name]) still worked, but the key never entered the order/memo maps, so a freshly required module was invisible to `for k in pairs(package.loaded)`. This was fixed on the perf/table-hash-iteration branch but the PR (#349) was squash-merged from a stale head that predated the fix, shipping the bug to main. Re-applies the one-line stdlib fix plus the pairs-visibility regression test, the table-iteration memo-invalidation/replace_data coverage, and the pairs_hash benchmark, all of which were stranded on the merged branch. Co-authored-by: Dave Lucia --- benchmarks/table_ops.exs | 27 +++++++++ lib/lua/vm/stdlib.ex | 2 +- test/lua/vm/stdlib/package_test.exs | 22 +++++++ test/lua/vm/table_iteration_test.exs | 90 ++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 1 deletion(-) diff --git a/benchmarks/table_ops.exs b/benchmarks/table_ops.exs index 6dd3243e..684bf376 100644 --- a/benchmarks/table_ops.exs +++ b/benchmarks/table_ops.exs @@ -5,6 +5,11 @@ # - sort: sort a reverse-ordered array (worst case for naive sort) # - iterate: sum all values via ipairs # - map_reduce: build → square each element → sum (two passes) +# - pairs_hash: build a string-keyed (hash-arm) table and sum every value +# via a full `for k,v in pairs(t)` walk. This is the workload +# the memoized hash-iteration path targets: each `next` step +# is an O(1) order-index lookup, so the full walk is O(n) +# rather than the O(n^2) of a linear order-list rescan. # # Compares: # - This Lua implementation (eval with string, eval with pre-compiled chunk) @@ -68,6 +73,21 @@ function run_table_map_reduce(n) end return sum end + +function run_table_pairs_hash(n) + -- String keys force every entry into the hash arm, so the full pairs + -- walk drives next_entry's hash advance path (the O(1) order-index + -- lookup this benchmark exists to measure) rather than the array arm. + local t = {} + for i = 1, n do + t["k" .. i] = i + end + local sum = 0 + for _, v in pairs(t) do + sum = sum + v + end + return sum +end """ # --- This Lua implementation --- @@ -103,6 +123,12 @@ map_reduce_chunks = {label, {chunk, "return run_table_map_reduce(#{n})", n}} end) +pairs_hash_chunks = + Map.new(sizes, fn {label, n} -> + {chunk, _} = Lua.load_chunk!(lua, "return run_table_pairs_hash(#{n})") + {label, {chunk, "return run_table_pairs_hash(#{n})", n}} + end) + # --- Luerl --- luerl_state = :luerl.init() {:ok, _, luerl_state} = :luerl.do(table_def, luerl_state) @@ -148,5 +174,6 @@ bench.("Table Build", build_chunks, :run_table_build) bench.("Table Sort", sort_chunks, :run_table_sort) bench.("Table Iterate/Sum", sum_chunks, :run_table_sum) bench.("Table Map + Reduce", map_reduce_chunks, :run_table_map_reduce) +bench.("Table Pairs (hash)", pairs_hash_chunks, :run_table_pairs_hash) c_lua_cleanup.() diff --git a/lib/lua/vm/stdlib.ex b/lib/lua/vm/stdlib.ex index 9c6a54c9..875b942c 100644 --- a/lib/lua/vm/stdlib.ex +++ b/lib/lua/vm/stdlib.ex @@ -785,7 +785,7 @@ defmodule Lua.VM.Stdlib do {:tref, loaded_id} = get_package_loaded_ref(state) State.update_table(state, {:tref, loaded_id}, fn loaded_table -> - %{loaded_table | data: Map.put(loaded_table.data, modname, result)} + Table.put(loaded_table, modname, result) end) end diff --git a/test/lua/vm/stdlib/package_test.exs b/test/lua/vm/stdlib/package_test.exs index f0c815af..06ef701b 100644 --- a/test/lua/vm/stdlib/package_test.exs +++ b/test/lua/vm/stdlib/package_test.exs @@ -106,6 +106,28 @@ defmodule Lua.VM.Stdlib.PackageTest do assert {["a", "table"], _} = eval_with_path(code, tmp_dir) end + test "a newly required module is visible to pairs(package.loaded)", %{tmp_dir: tmp_dir} do + # cache_module_result/3 routes the write through Table.put/3 so the + # required module's key enters the order/memo maintenance and becomes + # reachable by `for k in pairs(package.loaded)`. A direct Table.data + # write would still pass key-access tests but leave the key invisible + # to iteration; this pins the iteration-visibility half of that fix. + File.write!(Path.join(tmp_dir, "mymod.lua"), """ + return { ok = true } + """) + + code = ~S""" + require("mymod") + local seen = false + for k in pairs(package.loaded) do + if k == "mymod" then seen = true end + end + return seen + """ + + assert {[true], _} = eval_with_path(code, tmp_dir) + end + test "require returns the cached value on second call", %{tmp_dir: tmp_dir} do # The module increments a global counter on each evaluation; with the # cache working, the counter should be 1 after two requires. diff --git a/test/lua/vm/table_iteration_test.exs b/test/lua/vm/table_iteration_test.exs index 47cbd3ed..73ad9d2a 100644 --- a/test/lua/vm/table_iteration_test.exs +++ b/test/lua/vm/table_iteration_test.exs @@ -204,6 +204,96 @@ defmodule Lua.VM.TableIterationTest do assert cleared.order_index assert cleared.order_arr == table.order_arr end + + test "inserting a new key mid-walk then resuming from a prior key still advances" do + table = + %Table{} + |> Table.put("a", 1) + |> Table.put("b", 2) + |> Table.flush_order() + + # Begin the walk, then insert a brand-new hash key. The insert nils the + # memo (order_index/order_arr), so resuming from the already-returned + # "a" must fall back to advance_past over merged_order — which still + # contains "a" — and find the next live key rather than raising. + assert Table.next_entry(table, nil) == {"a", 1} + + mutated = Table.put(table, "zzz", 99) + assert mutated.order_index == nil + assert mutated.order_arr == nil + + resumed = Table.next_entry(mutated, "a") + assert resumed != :invalid_key + assert resumed == {"b", 2} + end + + test "absorbing a parked hash key into the array nils the memo" do + # put(3) lands in the hash (border is at 1, so 3 is sparse). After + # flush_order the memo is live and 3 is part of order_arr/order_index. + # put(2) is a contiguous append that extends the border to 2, which + # then absorbs the parked key 3 into the array via drop_hash_key — a + # distinct memo-invalidation trigger from insert_hash and plain delete. + table = + %Table{} + |> Table.put(1, "a") + |> Table.put(3, "c") + |> Table.put("s", "ess") + |> Table.flush_order() + + assert table.order_index + assert table.order_arr + + absorbed = Table.put(table, 2, "b") + assert absorbed.order_index == nil + assert absorbed.order_arr == nil + + # The walk is correct both before reflush (list-based fallback) and + # after reflush (rebuilt memo): array keys 1,2,3 then the hash key "s". + assert Enum.map(walk(absorbed), &elem(&1, 0)) == [1, 2, 3, "s"] + assert Enum.map(walk(Table.flush_order(absorbed)), &elem(&1, 0)) == [1, 2, 3, "s"] + end + + test "replace_data drops the memo and walks the new key set exactly once" do + table = + %Table{} + |> Table.put(1, "a") + |> Table.put("old", "x") + |> Table.put("gone", "y") + |> Table.flush_order() + + assert table.order_index + assert table.order_arr + + # Wholesale replacement must reset the memo so a stale order_arr built + # from the previous contents can never leak into the new walk. + replaced = Table.replace_data(table, %{1 => "one", 2 => "two", "new" => "n"}) + assert replaced.order_index == nil + assert replaced.order_arr == nil + + walked = walk(Table.flush_order(replaced)) + keys = Enum.map(walked, &elem(&1, 0)) + + assert keys == [1, 2, "new"] + assert length(keys) == length(Enum.uniq(keys)) + refute "old" in keys + refute "gone" in keys + end + + test "first_hash_live(nil) skips a value-cleared leading key with the memo live" do + table = + %Table{} + |> Table.put("a", 1) + |> Table.put("b", 2) + |> Table.flush_order() + + # Clear the FIRST key in order_arr. The value-clear keeps the memo live + # (order_index stays set), so first_hash_live must scan past the now-dead + # leading "a" rather than returning it. + cleared = Table.put(table, "a", nil) + + assert cleared.order_index + assert Table.next_entry(cleared, nil) == {"b", 2} + end end describe "iteration properties (StreamData)" do