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.

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

Huge performance improvement easily possible #86

Open
TheEt1234 opened this issue Feb 3, 2025 · 2 comments
Open

Huge performance improvement easily possible #86

TheEt1234 opened this issue Feb 3, 2025 · 2 comments

Comments

@TheEt1234
Copy link

TheEt1234 commented Feb 3, 2025

The areas:canInterract function uses minetest.check_player_privs without caching the result, this function is put into core.is_protected

This means, that, if there were 500 core.is_protected calls per step, it would call minetest.check_player_privs 500 times wastefully, this is not ideal

Storing the result dramatically increases the performance of core.is_protected

1st test: without the modification
Test code used: //lua local t0 = os.clock(); for i=1,1000000 do core.is_protected(vector.new(0,0,0), "the entity") end; core.debug(os.clock() - t0)
Time: 36.27 seconds

2nd test:
Modification used:
areas/api.lua line 98, function is incomplete

local priv_cache = {}
core.register_globalstep(function()
	priv_cache = {}
end)
-- Checks if the area is unprotected or owned by you
function areas:canInteract(pos, name)
	if priv_cache[name] == true then
		return true
	elseif priv_cache[name] == nil then
		priv_cache[name] = minetest.check_player_privs(name, self.adminPrivs)
		if priv_cache[name] then
			return true
		end
	end

Test code used: //lua local t0 = os.clock(); for i=1,1000000 do core.is_protected(vector.new(0,0,0), "the entity") end; core.debug(os.clock() - t0) (the same)
Time: 3.828 seconds

a 10x improvement (hehe proud of myself!)

@TheEt1234 TheEt1234 changed the title Huge performance improvement Huge performance improvement easily possible Feb 3, 2025
@SmallJoker
Copy link
Member

SmallJoker commented Feb 8, 2025

This means, that, if there were 500 core.is_protected calls per step, it would call minetest.check_player_privs 500 times wastefully, this is not ideal

What's the origin of the "500 calls per step"? Do those happen during peak loads of a server, or is it a guessed value?
According to your benchmarks, a single core.is_protected call would take 36.27 µs, and with the optimization 3.828 µs. That's already quite fast.
I think such optimization proposal would be better applied to the engine (caching in builtin) so that other mods do not need to implement the same code. It is also important to implement a privilege callback function to mark the cached value as outdated (core.register_on_priv_(grant|revoke)).

@TheEt1234
Copy link
Author

This means, that, if there were 500 core.is_protected calls per step, it would call minetest.check_player_privs 500 times wastefully, this is not ideal

What's the origin of the "500 calls per step"? Do those happen during peak loads of a server, or is it a guessed value? According to your benchmarks, a single core.is_protected call would take 36.27 µs, and with the optimization 3.828 µs. That's already quite fast. I think such optimization proposal would be better applied to the engine (caching in builtin) so that other mods do not need to implement the same code. It is also important to implement a privilege callback function to mark the cached value as outdated (core.register_on_priv_(grant|revoke)).

well i was making my own trees (that probably needed way more calls than that), and saw that core.is_protected took a significant amount of time on jitprofiler (and no, core.is_area_protected wouldn't solve anything for me, ive made my own l-system interpreter and the trees can get arbitrarily large)

and yes, this would be better to be in builtin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants