| Time |
Nick |
Message |
| 00:16 |
|
user333_ joined #luanti-dev |
| 00:21 |
|
SFENCE joined #luanti-dev |
| 00:39 |
|
SFENCE joined #luanti-dev |
| 00:54 |
|
nekobit joined #luanti-dev |
| 01:29 |
|
behalebabo joined #luanti-dev |
| 02:35 |
|
ivanbu joined #luanti-dev |
| 03:00 |
|
SFENCE_arch joined #luanti-dev |
| 03:51 |
|
user333_ joined #luanti-dev |
| 03:52 |
|
SFENCE joined #luanti-dev |
| 03:55 |
|
user333_ joined #luanti-dev |
| 05:00 |
|
MTDiscord joined #luanti-dev |
| 06:30 |
|
user333_ joined #luanti-dev |
| 07:31 |
|
pgimeno joined #luanti-dev |
| 08:26 |
|
ivanbu joined #luanti-dev |
| 08:38 |
|
lhofhansl joined #luanti-dev |
| 08:40 |
lhofhansl |
Why is the "loopcount >= initial_size" check needed here https://github.com/luanti-org/luanti/blob/master/src/servermap.cpp#L944? |
| 08:41 |
lhofhansl |
I added more instrumentation to map-loading and found that repeatedly processing the same liquid queue is causing the severe slowdown. |
| 08:42 |
lhofhansl |
Removing that part gets back to expected loading speeds. I get we do not want to do unbound processing, but perhaps we can limit this other ways. |
| 08:47 |
sfan5 |
I assume it's a rate-limit, because it makes sure the queue is fully looked at at most once |
| 08:51 |
sfan5 |
I think it's an oversight to not consider that initial_size may be very small |
| 08:51 |
sfan5 |
so `loopcount >= std::max(100, initial_size)` is probably better |
| 08:57 |
lhofhansl |
During mapgen of some ocean heavy scene, initial size is often around 1600, and the whole processed queue can be up 54000. |
| 08:58 |
lhofhansl |
(That's for a whole chunk of 512000 nodes) |
| 09:00 |
sfan5 |
well if this is happening on the emerge thread this limit should maybe not apply |
| 09:02 |
lhofhansl |
When I measure the max liquid queue processing time (after removing that check) it never goes above 40ms on my machine, (Average settles somewhere between 5 and 15 ms) |
| 09:03 |
lhofhansl |
Liquid processing is done on the main thread. |
| 09:03 |
sfan5 |
in the emerge code, whatever |
| 09:03 |
sfan5 |
but won't that lag the server anyway, since the env lock is held? |
| 09:04 |
lhofhansl |
transformLiquids is called from AsyncRunStep |
| 09:05 |
lhofhansl |
So, yes, it would lag the server more, but would allow for the maploading to not stall (in a busy loop, looking at the same blocks over and over, until the older blocks in the queue are finally processed). |
| 09:06 |
lhofhansl |
That's the tradeoff. |
| 09:06 |
sfan5 |
you mean map sending, not loading. still this is cool in singleplayer but not so cool in multiplayer |
| 09:06 |
sfan5 |
you don't want to lag the server for everyone to speed up map loading for one guy |
| 09:07 |
lhofhansl |
RemoteClient::getNextBlocks, yeah, sending. |
| 09:08 |
lhofhansl |
Could check for number of clients. |
| 09:11 |
lhofhansl |
It's just dumb how much CPU Luanti wastes, due to resetting the resent distance, and checking already sent blocks again. |
| 09:16 |
lhofhansl |
Actually... Normal liquid queue processing goes up to 30ms on my machine (with the check in place). So this won't actually increase the lag a lot. To be safe we could lower liquid_queue_max bit. |
| 09:18 |
lhofhansl |
Wait... Sorry, had the log still in there. Without any changes liquid processing is about 6ms on my machine. |
| 09:21 |
sfan5 |
two thoughts: 1. maybe we could be smarter about the reset send distance thing in general |
| 09:21 |
sfan5 |
2. but also could the time limit spent on liquids be made more dynamic without leading to more lag? |
| 09:23 |
lhofhansl |
Definitely yes on 1. but that stuff is subtle. (I opened and closed various PRs on that :) ). 2. Yes, we just need to decide what is acceptable lag. Maybe instead of size bound, we can time bound liquid processing. |
| 09:23 |
sfan5 |
3. it could also be possible to implement a "safe liquid scan" (that is thread-safe). it would only do things that can be done without lua/env callbacks. the idea is that this could be used in the emerge thread to get rid of most of the 54000 queue items, before doing the "critical" processing in the server thread |
| 09:27 |
sfan5 |
I think for your PR it's best not to try too many things at once. So I'd suggest taking the wins from the local liquid queue, leaving the rest alone, and thinking about it in follow-up PRs. |
| 09:44 |
lhofhansl |
Yeah... The local liquid queue for generated chunks only... But I'm not sure that works as I expected. I was looking at the queue process limits as an alternative. |
| 09:45 |
lhofhansl |
That local queue has to be processed completely, or changes are lost. |
| 09:54 |
sfan5 |
there is no need to lose the changes |
| 09:56 |
sfan5 |
re 3: transformLiquids looks completely safe unless it decides that a node must change. that could be factored out, or a new implementation that implements "if source liquid surrounded by same sources, remove from queue" also sounds like an useful first pass that can be done thread-safely |
| 10:00 |
lhofhansl |
I'll push a new update to the PR... Processing the whole queue for one chunk is quick (can't measure the difference actually). |
| 10:02 |
lhofhansl |
have a look |
| 10:10 |
lhofhansl |
I'll measure the maximal local chunk queue/loop count. |
| 10:13 |
lhofhansl |
Since accumulated over 1s it would never go above 55k, and it could process 70 chunks/s (in my test) the per chunk max queue size should be less than 800. |
| 10:22 |
sfan5 |
fwiw I did a quick test with skipping all liquid processing and while it makes the terrain loading noticably faster, it's not that slow without |
| 10:52 |
lhofhansl |
It's very pronounced when running with a local server (which uses dedicated server step of 90ms.) and with larger viewing_range. Notice how a newly generated map just sites there... For many, many seconds, apparently doing nothing. |
| 10:52 |
lhofhansl |
I added your suggestions. |
| 10:55 |
lhofhansl |
There is still some unnecessary waiting where there's a mix of loaded and generated blocks, but it's much better. Hopefully an easy win. |
| 10:56 |
lhofhansl |
As for thread-safety. transformLiquids accesses the map (getNode), so has to be under the envlock anyway. |
| 11:00 |
sfan5 |
local liquid updates could be done with just the MapBlock/VManip and queue |
| 11:01 |
lhofhansl |
And then record effects to neighboring blocks somewhere and process in the global queue? |
| 11:07 |
sfan5 |
since liquid updates also cause Lua calls this is only feasible to clean the queue of nodes that don't change. but yes, the rest would go into the global queue |
| 11:07 |
sfan5 |
<+sfan5> re 3: transformLiquids looks completely safe unless it decides that a node must change. that could be factored out, or a new implementation that implements "if source liquid surrounded by same sources, remove from queue" also sounds like an useful first pass that can be done thread-safely |
| 11:07 |
sfan5 |
maybe ReflowScanner already skips these, dunno |
| 11:11 |
Krock |
I find it interesting that the first thing I see in transformLiquids is that the positions are contained in a std::vector instead of std::set to avoid potential duplicates (if the code even allows that) |
| 11:12 |
sfan5 |
m_transforming_liquid is an UniqueQueue<v3s16> |
| 11:12 |
Krock |
oh I see. that's good. |
| 11:49 |
|
YuGiOhJCJ joined #luanti-dev |
| 13:05 |
|
user333_ joined #luanti-dev |
| 13:18 |
|
user333_ joined #luanti-dev |
| 13:28 |
|
user333_ joined #luanti-dev |
| 13:35 |
|
SFENCE_arch joined #luanti-dev |
| 13:59 |
|
Sheriff_U3 joined #luanti-dev |
| 15:11 |
|
Sheriff_U3 joined #luanti-dev |
| 15:43 |
|
SFENCE joined #luanti-dev |
| 16:41 |
|
Desour joined #luanti-dev |
| 17:51 |
|
SFENCE joined #luanti-dev |
| 19:51 |
|
fluxionary joined #luanti-dev |
| 19:55 |
|
SFENCE joined #luanti-dev |
| 20:09 |
|
SFENCE joined #luanti-dev |
| 20:09 |
|
user333_ joined #luanti-dev |
| 20:10 |
|
user333_ joined #luanti-dev |
| 20:13 |
|
user333_ joined #luanti-dev |
| 23:17 |
|
mrcheese joined #luanti-dev |
| 23:30 |
|
SFENCE joined #luanti-dev |
| 23:36 |
|
panwolfram joined #luanti-dev |
