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

[MANUAL MERGE] Async environment for mods to do concurrent tasks #11131

Closed
wants to merge 21 commits into from

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Mar 28, 2021

basically #3375 by integrating the existing mechanism of async jobs that the mainmenu uses

make sure to read the new section in lua_api.txt

Merging

  1. Keep the first four commits intact
  2. Squash the rest into a single one named Async environment for mods to do concurrent tasks (#11131)

To do

This PR is Ready for Review.

  • make compatible with mod security
  • allow mods to register stuff to be imported at env init time
  • allow certain builtin objects to be transferred in and out of the env (VoxelManip, ItemStack, ...)
  • automatically scale number of worker threads
  • check APIs to see which we can expose in async env
  • transfer core.registered_* into async env

Ideas:

  • allow mods to get a worker just for themselves? (for sync purposes and longer running tasks)
  • function to poll for job completion

How to test

Play around with/try the following

  • /a: Execute any code in any async env, shows return values
    /return: The same but return is prepended for convenience (e.g. /return _VERSION, type(_G), jit and jit.arch)
core.register_chatcommand("a", {
	privs = {server = true},
	func = function(name, param)
		local func, err = loadstring("return function(name)\n" .. param .. "\nend")
		if not func then
			return true, err
		end
		core.handle_async(func(), function(...)
			local ret = {...}
			if #ret == 1 and type(ret[1]) == "string" then
				ret = ret[1]
			else 
				ret = #ret < 2 and dump(ret[1]) or dump(ret)
			end
			core.chat_send_player(name, "result: " .. ret)
		end, name)
		return true
	end,
})
core.register_chatcommand("return", {
	privs = {server = true},
	func = function(name, param)
		return core.registered_chatcommands["a"].func(name, "return " .. param)
	end,
})
  • /err 1 or /err 2: tests how errors inside the async env or return callback are reported
core.register_chatcommand("err", {
	privs = {server = true},
	func = function(name, param)
		if param == "1" then
			core.handle_async(function()
				error("inside async")
			end, function() end)
		elseif param == "2" then
			core.handle_async(function() end, function()
				error("inside callback")
			end)
		end
		return true
	end,
})
  • Just for fun: /bitcoin run an intensive background task based on the same principle as Bitcoin mining,
    which is finding hashes with chosen prefixes (null bytes)
core.register_chatcommand("bitcoin", {
	privs = {server = true},
	func = function(name, param)
		local function f(prefix)
			local r = PcgRandom(core.get_us_time() + math.floor(math.random() * 1000000))
			local v = {}
			local i = 1
			while true do
				for j = 1, 256 do
					v[j] = r:next()
				end
				local h = core.sha1(table.concat(v, ""))
				if h:sub(1, #prefix) == prefix then
					return string.format("Found %s after %d iterations", h, i)
				end
				i = i + 1
			end
		end
		for w = 1, tonumber(param) or 1 do
			core.handle_async(f, function(s)
				core.chat_send_all(core.colorize('#1d1', ">> ") .. s)
			end, "0000")
		end
		return true, "Job(s) submitted."
	end,
})
  • /flip flips a mapblocks but does the computation in another thread (worldedit required)
core.register_chatcommand("flip", {
	privs = {worldedit = true},
	func = function(name, param)
		assert(minetest.global_exists("worldedit"))
		if not worldedit.pos1[name] then
			return false, "Need to select pos1"
		end
		local vm_ = core.get_voxel_manip(worldedit.pos1[name], worldedit.pos1[name])
		core.handle_async(function(vm)
			local data = vm:get_data()
			local tmp
			for i = 1, 2048 do
				tmp = data[4097-i]
				data[4097-i] = data[i]
				data[i] = tmp
			end
			vm:set_data(data)
			vm:get_param2_data(data)
			for i = 1, 2048 do
				tmp = data[4097-i]
				data[4097-i] = data[i]
				data[i] = tmp
			end
			vm:set_param2_data(data)
			return vm
		end, function(vm)
			vm:write_to_map()
			core.chat_send_player(name, "Success :D")
		end, vm_)
		return true
	end,
})

@sfan5 sfan5 added @ Server / Client / Env. WIP Feature ✨ PRs that add or enhance a feature labels Mar 28, 2021
@sfan5
Copy link
Member Author

sfan5 commented Mar 28, 2021

Feedback wanted

If you have a mod that could benefit from offloading tasks into background thread(s) please take a look at this PR and give feedback if/how the current API would fit.
You're welcome to try it out and see how it actually works, too.

--------------------

nerzhul
nerzhul previously requested changes Apr 2, 2021
Copy link
Member

@nerzhul nerzhul left a comment

Choose a reason for hiding this comment

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

sounds to be a great start. Can't we do the whole async job from C++ instead of going to lua (i mean the core.job_processor part)

builtin/async/init.lua Outdated Show resolved Hide resolved
@lhofhansl
Copy link
Contributor

@sfan5 Would that be usable to do Lua MapGen without holding the big env lock? (I don't think so, but wanted to make sure)

@sfan5
Copy link
Member Author

sfan5 commented Apr 5, 2021

You could push mapgen tasks to async workers inside on_generated and then copy the result to the map once they're done but this has several problems and is a poor imitation of the real solution. So, no.

@Desour
Copy link
Member

Desour commented Apr 5, 2021

  • Wouldn't it be easier if there was just some low-level ways of creating new threads and do IPC?
    The high level stuff, like the abstraction with async jobs, could then be implemented as helpers in builtin.
    This would give mods more room to do specialized stuff, or try to implement better async apis. And it might make the implementation simpler.

  • It might be easier to not dump the async function, but instead directly give the lua code as string. The current way brings some unintuitive problems with it, stuff that normally works in lua (ie. using upvalues and globals) suddenly doesn't work anymore. Just having a string would make things clear.

  • It might be easier to first register some function and then asyncly call that function by its name, similar to mesecons' actionqueue's functions and actions (see https://github.com/minetest-mods/mesecons/blob/master/mesecons/actionqueue.lua). This would also remove the need of loading new lua code every time when you start an async job (note: I don't know how expensive loading lua code is).

@sfan5
Copy link
Member Author

sfan5 commented Apr 9, 2021

Wouldn't it be easier if there was just some low-level ways of creating new threads and do IPC? [...] And it might make the implementation simpler.

Ordinarily yes, but in this case, no. This implementation is as simple as it gets since the async job code already exists.
I'm also not sure if mods need low-level control, providing this simpler API might be overall better since each mod won't be spawning 5 threads to do minor things once in a while.

The current way brings some unintuitive problems with it, stuff that normally works in lua (ie. using upvalues and globals)

Uh? Globals work fine and upvales can, by definition, not work when transferred into another Lua environment.

It might be easier to first register some function and then asyncly call that function by its name. This would also remove the need of loading new lua code every time when you start an async job.

That's planned (the "• allow mods to register stuff to be imported at env init time" point).

@Desour
Copy link
Member

Desour commented Apr 9, 2021

Uh? Globals work fine and upvales can, by definition, not work when transferred into another Lua environment.

But you are using different globals. My point was that it's weird that you give a function as argument, which kinda looks like you could use everything that you normally use in a function. A string would make the difference clearer.

@appgurueu
Copy link
Contributor

Might be useful for transaction log files. Those should be rewritten every now and then, which should ideally happen in the background. A separate process could be spawned, but that's quite a workaround.

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author label May 25, 2021
@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author label Aug 26, 2021
builtin/async/init.lua Outdated Show resolved Hide resolved
builtin/async/init.lua Outdated Show resolved Hide resolved
builtin/async/init.lua Outdated Show resolved Hide resolved
@appgurueu
Copy link
Contributor

I don't see why the (de)serialization overhead is necessary. As this uses a separate Lua state, copying Lua objects is of course necessary, but why can't this be done efficiently in C++ though?

@sfan5
Copy link
Member Author

sfan5 commented Aug 28, 2021

Copying the data directly between Lua states is tricky but possible if:

  • The other state is not busy executing Lua (you need to lock it)
  • You know ahead of time which worker will get the job

For anything else an intermediate format is necessary.
I'd rather move the serialization to C++ and probably have to anyway so that userdata things can properly and safely transfer their data.

@TurkeyMcMac
Copy link
Contributor

One feature that would be useful but that I don't see in the API is a way to wait for a particular task to finish. This would allow for fork-join type multithreaded code.

@sfan5
Copy link
Member Author

sfan5 commented Jan 22, 2022

Wouldn't this be equivalent?

local results = {}
local finish = function(ret)
	results[#results+1] = ret
	if #results == 5 then
		-- do something with `results`
	end
end
for i = 1,5 do
	minetest.handle_async(function(obj)
		-- do something with `obj`
		return obj
	end, finish, {foo="bar"})
end

If by "wait" you meant a blocking wait then that's not really possible but you can use coroutines to emulate this.

@TurkeyMcMac
Copy link
Contributor

I did mean blocking. Ideally, doing computation in parallel should be an implementation detail of mods. It should not require redesigning the mods' code to be asynchronous. Blocking seems like it should be theoretically possible, although it might require changing a lot of the internal code.

@appgurueu
Copy link
Contributor

I did mean blocking. Ideally, doing computation in parallel should be an implementation detail of mods. It should not require redesigning the mods' code to be asynchronous. Blocking seems like it should be theoretically possible, although it might require changing a lot of the internal code.

Couldn't you trivially implement blocking using a busywait? In sfan's code, repeat until #results == 5.

@TurkeyMcMac
Copy link
Contributor

I think the results of async jobs are only polled between ticks.

@rubenwardy
Copy link
Member

Blocking would remove the benefit of concurrency. Perhaps you want coroutines ?

sfan5 added 16 commits April 30, 2022 16:14
If a newly started thread immediately exits then m_running would
immediately be set to false again and the caller would be stuck
waiting for m_running to become true forever.
Since a mutex for synchronizing startup already exists we can
simply move the while loop into it.

see also: minetest#5134 which introduced m_start_finished_mutex
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Tested the provided /flip command and the devtest code. Works.

No newly introduced memory leaks found. 2x Conditional jump or move depends on uninitialised value(s) which might be Lua false-positives.

src/server.cpp Outdated Show resolved Hide resolved
@sfan5
Copy link
Member Author

sfan5 commented May 2, 2022

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Server / Client / Env. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants