Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some unneeded memory allocation #614

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Remove some unneeded memory allocation #614

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fee53f3
Speed up internal info getters a little bit
TurkeyMcMac May 19, 2022
468ec52
Improve linking
TurkeyMcMac May 20, 2022
d4acc6a
Avoid most node copies
TurkeyMcMac May 20, 2022
569677d
Fix use of link_inverted
TurkeyMcMac May 20, 2022
49645b1
Rename const_node to rule_node_nocopy
TurkeyMcMac May 29, 2022
55cc507
Improve non-VM swap_node_force
TurkeyMcMac May 29, 2022
e4f2f84
Implement legacy linking functions
TurkeyMcMac Jul 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions mesecons/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ mesecon.queue:add_function("receptor_on", function (pos, rules)
-- Call turnon on all linking positions
for _, rule in ipairs(mesecon.flattenrules(rules)) do
local np = vector.add(pos, rule)
local rulenames = mesecon.rules_link_rule_all(pos, rule)
for _, rulename in ipairs(rulenames) do
local rulename = mesecon.link(pos, np)
if rulename then
mesecon.turnon(np, rulename)
end
end
Expand All @@ -96,8 +96,8 @@ mesecon.queue:add_function("receptor_off", function (pos, rules)
-- Call turnoff on all linking positions
for _, rule in ipairs(mesecon.flattenrules(rules)) do
local np = vector.add(pos, rule)
local rulenames = mesecon.rules_link_rule_all(pos, rule)
for _, rulename in ipairs(rulenames) do
local rulename = mesecon.link(pos, np)
if rulename then
mesecon.vm_begin()
mesecon.changesignal(np, minetest.get_node(np), rulename, mesecon.state.off, 2)

Expand Down
181 changes: 80 additions & 101 deletions mesecons/internal.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,48 +50,59 @@
mesecon.fifo_queue = dofile(minetest.get_modpath("mesecons").."/fifo_queue.lua")

-- General
function mesecon.get_effector(nodename)
if minetest.registered_nodes[nodename]
and minetest.registered_nodes[nodename].mesecons
and minetest.registered_nodes[nodename].mesecons.effector then
return minetest.registered_nodes[nodename].mesecons.effector

local function get_mesecons_spec(nodename)
local def = minetest.registered_nodes[nodename]
return def and def.mesecons
end

local function get_rules(def, node)
local rules = def.rules
if type(rules) == 'function' then
if not def.const_node then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you happen to know where this would be documented?

Also: How much of a performance difference are we talking about here? A new variable and checks to skip a table creation with three indices seems to be overkill - at least to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where this should be documented. I can't tell the source of the documentation at https://mesecons.net/developers.html.

I think avoiding copying is worth it, since the garbage collector is a known limitation to the speed of LuaJIT. If you have 10k insulated wire segments, that's 10k allocations saved.

Copy link
Member

@SmallJoker SmallJoker May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that code complexity (O) matters most. If the function calls are proportional to the amount of placed wires, such an optimization does not seem to be urgent due to "sane" linear scaling in computational. Of course it does make a small difference, but nothing compared to the case if it would scale by O(n²). Hence the time used by an allocation and later GC appears negligible.

Either way ... what would you think of renaming the variable to rule_node_nocopy? action_on still receives a copy after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimizations to the code complexity have the highest yield, but eventually the only optimizations left to do involve reducing the constant factor. I think these are still important since people use Mesecons to build vast contraptions such as whole computers. In these cases, even a 1% optimization reduces the absolute execution time by a lot.

I will rename the variable to rule_node_nocopy.

-- Copy the node to avoid overwriting data in the cache
node = {name = node.name, param1 = node.param1, param2 = node.param2}
end
return rules(node)
elseif rules then
return rules
end
return mesecon.rules.default
end

function mesecon.get_effector(nodename)
local mesecons_spec = get_mesecons_spec(nodename)
return mesecons_spec and mesecons_spec.effector
end

function mesecon.get_receptor(nodename)
if minetest.registered_nodes[nodename]
and minetest.registered_nodes[nodename].mesecons
and minetest.registered_nodes[nodename].mesecons.receptor then
return minetest.registered_nodes[nodename].mesecons.receptor
end
local mesecons_spec = get_mesecons_spec(nodename)
return mesecons_spec and mesecons_spec.receptor
end

function mesecon.get_conductor(nodename)
if minetest.registered_nodes[nodename]
and minetest.registered_nodes[nodename].mesecons
and minetest.registered_nodes[nodename].mesecons.conductor then
return minetest.registered_nodes[nodename].mesecons.conductor
end
local mesecons_spec = get_mesecons_spec(nodename)
return mesecons_spec and mesecons_spec.conductor
end

function mesecon.get_any_outputrules(node)
if not node then return nil end

if mesecon.is_conductor(node.name) then
return mesecon.conductor_get_rules(node)
elseif mesecon.is_receptor(node.name) then
return mesecon.receptor_get_rules(node)
end
if not node then return end
local mesecons_spec = get_mesecons_spec(node.name)
if not mesecons_spec then return end
local conductor = mesecons_spec.conductor
if conductor then return get_rules(conductor, node) end
local receptor = mesecons_spec.receptor
if receptor then return get_rules(receptor, node) end
end

function mesecon.get_any_inputrules(node)
if not node then return nil end

if mesecon.is_conductor(node.name) then
return mesecon.conductor_get_rules(node)
elseif mesecon.is_effector(node.name) then
return mesecon.effector_get_rules(node)
end
if not node then return end
local mesecons_spec = get_mesecons_spec(node.name)
if not mesecons_spec then return end
local conductor = mesecons_spec.conductor
if conductor then return get_rules(conductor, node) end
local effector = mesecons_spec.effector
if effector then return get_rules(effector, node) end
end

function mesecon.get_any_rules(node)
Expand Down Expand Up @@ -127,16 +138,7 @@ end

function mesecon.receptor_get_rules(node)
local receptor = mesecon.get_receptor(node.name)
if receptor then
local rules = receptor.rules
if type(rules) == 'function' then
return rules(node)
elseif rules then
return rules
end
end

return mesecon.rules.default
return receptor and get_rules(receptor, node) or mesecon.rules.default
end

-- Effectors
Expand Down Expand Up @@ -167,15 +169,7 @@ end

function mesecon.effector_get_rules(node)
local effector = mesecon.get_effector(node.name)
if effector then
local rules = effector.rules
if type(rules) == 'function' then
return rules(node)
elseif rules then
return rules
end
end
return mesecon.rules.default
return effector and get_rules(effector, node) or mesecon.rules.default
end

-- #######################
Expand Down Expand Up @@ -340,29 +334,21 @@ end

function mesecon.conductor_get_rules(node)
local conductor = mesecon.get_conductor(node.name)
if conductor then
local rules = conductor.rules
if type(rules) == 'function' then
return rules(node)
elseif rules then
return rules
end
end
return mesecon.rules.default
return conductor and get_rules(conductor, node) or mesecon.rules.default
end

-- some more general high-level stuff

function mesecon.is_power_on(pos, rulename)
local node = mesecon.get_node_force(pos)
local node = mesecon.get_node_force_nocopy(pos)
if node and (mesecon.is_conductor_on(node, rulename) or mesecon.is_receptor_on(node.name)) then
return true
end
return false
end

function mesecon.is_power_off(pos, rulename)
local node = mesecon.get_node_force(pos)
local node = mesecon.get_node_force_nocopy(pos)
if node and (mesecon.is_conductor_off(node, rulename) or mesecon.is_receptor_off(node.name)) then
return true
end
Expand Down Expand Up @@ -428,7 +414,7 @@ function mesecon.turnon(pos, link)

local depth = 1
for f in frontiers:iter() do
local node = mesecon.get_node_force(f.pos)
local node = mesecon.get_node_force_nocopy(f.pos)

if not node then
-- Area does not exist; do nothing
Expand All @@ -441,7 +427,8 @@ function mesecon.turnon(pos, link)
for _, r in ipairs(mesecon.rule2meta(f.link, rules)) do
local np = vector.add(f.pos, r)
if not pos_can_be_skipped[minetest.hash_node_position(np)] then
for _, l in ipairs(mesecon.rules_link_rule_all(f.pos, r)) do
local l = mesecon.link(f.pos, np)
if l then
frontiers:add({pos = np, link = l})
end
end
Expand Down Expand Up @@ -492,7 +479,7 @@ function mesecon.turnoff(pos, link)

local depth = 1
for f in frontiers:iter() do
local node = mesecon.get_node_force(f.pos)
local node = mesecon.get_node_force_nocopy(f.pos)

if not node then
-- Area does not exist; do nothing
Expand All @@ -508,14 +495,15 @@ function mesecon.turnoff(pos, link)
-- Check if an onstate receptor is connected. If that is the case,
-- abort this turnoff process by returning false. `receptor_off` will
-- discard all the changes that we made in the voxelmanip:
if mesecon.rules_link_rule_all_inverted(f.pos, r)[1] then
if mesecon.is_receptor_on(mesecon.get_node_force(np).name) then
if mesecon.link_inverted(f.pos, np) then
if mesecon.is_receptor_on(mesecon.get_node_force_nocopy(np).name) then
return false
end
end

-- Call turnoff on neighbors
for _, l in ipairs(mesecon.rules_link_rule_all(f.pos, r)) do
local l = mesecon.link(f.pos, np)
if l then
frontiers:add({pos = np, link = l})
end
end
Expand Down Expand Up @@ -551,48 +539,39 @@ function mesecon.turnoff(pos, link)
return true
end

-- Get all linking inputrules of inputnode (effector or conductor) that is connected to
-- outputnode (receptor or conductor) at position `output` and has an output in direction `rule`
function mesecon.rules_link_rule_all(output, rule)
local input = vector.add(output, rule)
local inputnode = mesecon.get_node_force(input)
-- Get the linking inputrule of inputnode (effector or conductor) that is connected to
-- outputnode (receptor or conductor) between positions `output` and `input`
function mesecon.link(output, input)
local inputnode = mesecon.get_node_force_nocopy(input)
local inputrules = mesecon.get_any_inputrules(inputnode)
if not inputrules then
return {}
end
local rules = {}
if not inputrules then return end

local dx, dy, dz = output.x - input.x, output.y - input.y, output.z - input.z
for _, inputrule in ipairs(mesecon.flattenrules(inputrules)) do
-- Check if input accepts from output
if vector.equals(vector.add(input, inputrule), output) then
table.insert(rules, inputrule)
if inputrule.x == dx and inputrule.y == dy and inputrule.z == dz then
return inputrule
end
end

return rules
end

-- Get all linking outputnodes of outputnode (receptor or conductor) that is connected to
-- inputnode (effector or conductor) at position `input` and has an input in direction `rule`
function mesecon.rules_link_rule_all_inverted(input, rule)
local output = vector.add(input, rule)
local outputnode = mesecon.get_node_force(output)
-- Get the linking outputrule of outputnode (receptor or conductor) that is connected to
-- inputnode (effector or conductor) between positions `input` and `output`
function mesecon.link_inverted(input, output)
local outputnode = mesecon.get_node_force_nocopy(output)
local outputrules = mesecon.get_any_outputrules(outputnode)
if not outputrules then
return {}
end
local rules = {}
if not outputrules then return end

local dx, dy, dz = input.x - output.x, input.y - output.y, input.z - output.z
for _, outputrule in ipairs(mesecon.flattenrules(outputrules)) do
if vector.equals(vector.add(output, outputrule), input) then
table.insert(rules, mesecon.invertRule(outputrule))
if outputrule.x == dx and outputrule.y == dy and outputrule.z == dz then
return outputrule
end
end
return rules
end

function mesecon.is_powered(pos, rule)
local node = mesecon.get_node_force(pos)
local node = mesecon.get_node_force_nocopy(pos)
local rules = mesecon.get_any_inputrules(node)
if not rules then return false end

Expand All @@ -601,23 +580,23 @@ function mesecon.is_powered(pos, rule)

if not rule then
for _, rule in ipairs(mesecon.flattenrules(rules)) do
local rulenames = mesecon.rules_link_rule_all_inverted(pos, rule)
for _, rname in ipairs(rulenames) do
local np = vector.add(pos, rname)
local nn = mesecon.get_node_force(np)
local np = vector.add(pos, rule)
local rname = mesecon.link_inverted(pos, np)
if rname then
local nn = mesecon.get_node_force_nocopy(np)

if (mesecon.is_conductor_on(nn, mesecon.invertRule(rname))
if (mesecon.is_conductor_on(nn, rname)
or mesecon.is_receptor_on(nn.name)) then
table.insert(sourcepos, np)
end
end
end
else
local rulenames = mesecon.rules_link_rule_all_inverted(pos, rule)
for _, rname in ipairs(rulenames) do
local np = vector.add(pos, rname)
local nn = mesecon.get_node_force(np)
if (mesecon.is_conductor_on (nn, mesecon.invertRule(rname))
local np = vector.add(pos, rule)
local rname = mesecon.link_inverted(pos, np)
if rname then
local nn = mesecon.get_node_force_nocopy(np)
if (mesecon.is_conductor_on (nn, rname)
or mesecon.is_receptor_on (nn.name)) then
table.insert(sourcepos, np)
end
Expand Down
43 changes: 31 additions & 12 deletions mesecons/util.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,16 +390,21 @@ local function vm_get_or_create_entry(pos)
return tbl
end

-- Gets the node at a given position during a VoxelManipulator-based
-- transaction.
function mesecon.vm_get_node(pos)
local function vm_get_node_nocopy(pos)
local hash = minetest.hash_node_position(pos)
local node = vm_node_cache[hash]
if not node then
node = vm_get_or_create_entry(pos).vm:get_node_at(pos)
vm_node_cache[hash] = node
end
return node.name ~= "ignore" and {name = node.name, param1 = node.param1, param2 = node.param2} or nil
return node.name ~= "ignore" and node or nil
end

-- Gets the node at a given position during a VoxelManipulator-based
-- transaction.
function mesecon.vm_get_node(pos)
local node = vm_get_node_nocopy(pos)
return node and {name = node.name, param1 = node.param1, param2 = node.param2}
end

-- Sets a node’s name during a VoxelManipulator-based transaction.
Expand All @@ -424,6 +429,18 @@ function mesecon.vm_swap_node(pos, name, update_light)
tbl.dirty = true
end

-- Get node, loading map if necessary.
local function get_node_load(pos)
local node = minetest.get_node_or_nil(pos)
if node == nil then
-- Node is not currently loaded; use a VoxelManipulator to prime
-- the mapblock cache and try again.
minetest.get_voxel_manip(pos, pos)
node = minetest.get_node_or_nil(pos)
end
return node
end

-- Gets the node at a given position, regardless of whether it is loaded or
-- not, respecting a transaction if one is in progress.
--
Expand All @@ -435,14 +452,16 @@ function mesecon.get_node_force(pos)
if vm_cache then
return mesecon.vm_get_node(pos)
else
local node = minetest.get_node_or_nil(pos)
if node == nil then
-- Node is not currently loaded; use a VoxelManipulator to prime
-- the mapblock cache and try again.
minetest.get_voxel_manip(pos, pos)
node = minetest.get_node_or_nil(pos)
end
return node
return get_node_load(pos)
end
end

-- Same without copying the internal node. Not part of public API.
function mesecon.get_node_force_nocopy(pos)
if vm_cache then
return vm_get_node_nocopy(pos)
else
return get_node_load(pos)
end
end

Expand Down
Loading