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

Consolidate API object code #12728

Merged
merged 6 commits into from Oct 4, 2022

Conversation

TurkeyMcMac
Copy link
Contributor

@TurkeyMcMac TurkeyMcMac commented Aug 27, 2022

This PR reduces repetition in the code that implements userdata objects such as VoxelManip and Settings. Overall, it removes over 600 lines of code. It should also decrease the size of the executable, but I haven't measured that.

To do

This PR is Ready for Review.

How to test

There are many changes in the PR, and it's possible that some are erroneous. Someone should look through them to make sure this is not the case.

@sfan5
Copy link
Member

sfan5 commented Sep 2, 2022

Hmm, not sure if I like the closure/upvalue solution even if it is a bit faster. But we'll get to this when the other PR is done.

@TurkeyMcMac TurkeyMcMac force-pushed the consolidate-api-obj branch 2 times, most recently from ba21ee6 to dc9642c Compare September 15, 2022 00:47
@sfan5 sfan5 added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Sep 18, 2022
@TurkeyMcMac
Copy link
Contributor Author

I probably won't rebase this unless the concept is approved.

@Zughy Zughy added Rebase needed The PR needs to be rebased by its author. and removed Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Sep 26, 2022
@TurkeyMcMac
Copy link
Contributor Author

Nevermind, this rebase at least was easy.

@TurkeyMcMac TurkeyMcMac added @ Script API and removed Rebase needed The PR needs to be rebased by its author. labels Sep 28, 2022
@sfan5
Copy link
Member

sfan5 commented Sep 28, 2022

Okay so like I hinted at I'm not a fan of the extra complexity of the upvalue/closure solution and I think we should keep using luaL_checkudata.
In that case what should be done to reduce duplication is to turn checkobject into a templated helper and the same registerClass change as already done now.

@TurkeyMcMac
Copy link
Contributor Author

I have changed checkObject to be templated. That does clean up the code a bit. I have not removed methodCheckObject at the moment.

I should do a benchmark using PUC Lua where a RNG is used in a loop. This is a realistic situation to benchmark method overhead.

@TurkeyMcMac
Copy link
Contributor Author

TurkeyMcMac commented Sep 28, 2022

Realistic benchmark using PUC Lua:

local rng = PcgRandom(123)
local results = {}
local before = core.get_us_time()
for i = 1, 10000 do
	results[i] = rng:next()
end
local after = core.get_us_time()
core.debug("Benchmark:", after - before)

Time with methodCheckObject: ~1200
Time with checkObject: ~1600

@Desour
Copy link
Member

Desour commented Sep 28, 2022

All that luaL_checkudata does different is where it gets the class's userdata from:

LUALIB_API void *luaL_checkudata (lua_State *L, int ud, const char *tname) {
void *p = lua_touserdata(L, ud);
if (p != NULL) { /* value is a userdata? */
if (lua_getmetatable(L, ud)) { /* does it have a metatable? */
lua_getfield(L, LUA_REGISTRYINDEX, tname); /* get correct metatable */
if (lua_rawequal(L, -1, -2)) { /* does it have the correct mt? */
lua_pop(L, 2); /* remove both metatables */
return p;
}
}
}
luaL_typerror(L, ud, tname); /* else error */
return NULL; /* to avoid warnings */
}

You could try to store the userdatas in an array table in the registry instead of using a string index.

@TurkeyMcMac
Copy link
Contributor Author

Good idea.

@TurkeyMcMac
Copy link
Contributor Author

With LuaJIT, the integer index solution seemed like a lot of complexity for no performance gain, perhaps even a small performance loss. I decided to remove the optimizations from this PR altogether. They can be added separately.

@sfan5
Copy link
Member

sfan5 commented Oct 3, 2022

This really should not be micro-optimized, with the numbers from the earlier benchmark the idiomatic solution with luaL_checkudata is 40 nanoseconds slower than upvalues.

@TurkeyMcMac
Copy link
Contributor Author

Probably true. In any case, optimization is no longer a goal of this PR.

src/script/lua_api/l_base.h Outdated Show resolved Hide resolved
src/script/lua_api/l_metadata.cpp Show resolved Hide resolved
@TurkeyMcMac TurkeyMcMac requested a review from sfan5 October 4, 2022 11:17
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

LGTM
if you've tested it reasonably feel free to merge

@TurkeyMcMac TurkeyMcMac merged commit 7632af3 into minetest:master Oct 4, 2022
appgurueu pushed a commit to appgurueu/minetest that referenced this pull request May 2, 2023
Co-authored-by: sfan5 <sfan5@live.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants