Monoblocks: optimize blocks that contain a single type of node

[lhofhansl]

(This continues #16152. Lot's discussion there, but I cannot reopen that one, because I force pushed a rebase before I opened it. There a way to fix that, but it seemed easier to open a new PR, and it's what github wants.)
(And apparently I am an idiot. I meant to update a message, and accidentally closed #16291, and then force-pushed a change. This time github was really mad at me... So here is attempt 3 with a brand-new branch, hopefully the last one.)

See discussion in #16097

For blocks that only contain a single type of node, that node is only stored once.
When copied to a VMANIP the full array is materialized. Could be optimized.

@appgurueu pointed out, rightly, that the normal compression we do in disk and on the wire naturally already takes care of mono-block, so this is just to save in process memory.

Only works on the server, currently. Making it work on the client would need a protocol version change.

• Goal of the PR, How does the PR work?
  See description

• Does it resolve any reported issue?
  Fixes Standing still increases ram consumption #16097

To do

This PR is a ready for review.

It needs mostly testing and a review of the approach.

How to test

Try any world, new or existing, everything should just work.
There should be no measurable overall slowdown, and server memory usage should be drastically (50-90% depending on scene) reduced, especially with larger viewing ranging.

Try with many mesh-generation-threads and larger values of client_mesh_chunk.

[lhofhansl]

It occurs to me that the MapNode [] on the client will never change. It always stays in its expanded state (we never call a method on the client that will try to shrink the array.)

So maybe we focus on threading correctness changes on the server.
(And if I close this issue again, by accident, I'm going to ... )

[lhofhansl]

Did some more tests:
Scene with land and water, viewing range 2000 (and all settings adjusted accordingly). When fully loaded the server has about 1m blocks loaded, the client about 20k blocks.

Without the PR server memory usage is > 16GB, with the PR around 1GB. So a 16x improvement.

(When I enabled this on the client too - by calling tryShrinkNodes when a block is deserialized from the network - it made no measurable difference. 3.6GB in both cases. Presumably because most air blocks are not sent to the client anyway.)

Imagine you have 5 or 10 players at this range.

[lhofhansl]

Would it help if I put this behind an "experimental" setting, or perhaps a compile time option?

[farooqkz]

Would it help if I put this behind an "experimental" setting, or perhaps a compile time option?

I would vote for experimental setting/property rather than a compile time option. This is from server admin and player point of view. I'm not into engine development. The reason is that when someone wants to just host a server from the GUI and if they have a lowend hardware, they'll have to compile from source which even for developers is very inconvenient. Let alone kids who just want to play.

[lhofhansl]

I looked again at the code. All mapgen is triggered from the EmergeThread(s).

• EmergeThread::getBlockOrStartGen locks the envlock
• EmergeThread::finishGen locks the envlock (this calls ServerMap::finishBlockMake to move the generated data into the map - still holding the envlock)
• m_mapgen->makeChunk(&bmdata) has no access to map or blocks
• m_script->on_generated(&bmdata, m_mapgen->blockseed); has no access to map or blocks
• event dispatching is also done under the envlock

MeshUpdateQueue::fillDataFromMapBlocks called from MeshUpdateQueue::pop is the only incorrect access I could find (and it's on the client, and on the client there are no calls that will ever shrink or expand the internal MapNode [])

So we really should be good. Of course I could have missed something. It's hard/impossible to prove the absence of something. @sfan5

[sfan5]

That sounds good, except

MeshUpdateQueue::fillDataFromMapBlocks called from MeshUpdateQueue::pop is the only incorrect access I could find (and it's on the client, and on the client there are no calls that will ever shrink or expand the internal MapNode [])

..is a footgun waiting to happen. It doesn't look like the Client, ClientEnvironment or ClientMap has a lock, it probably should and then we could just take it for the duration of the copy.

[lhofhansl]

..is a footgun waiting to happen.

Do you mean this as a general foot gun, or specifically if somebody later changes how Mono-Blocks work on the client?

Generally, yes... And that is a different issue. This PR will not make this better or worse. Maybe I add comments at the right spots. I can probably distinguish a MapBlock on the client by using the used IGameDef, and then simply disallow any tryShrinkNodes action.
(Or a ClientMapBlock and ServerMapBlock, like ClientMap and ServerMap).

Locking the client during the copy would effectively be like #16282, with the same performance profile (at least I cannot think of any scenario that would not lock out the client while we copy the block data to the VManip). We need to think about that more. Let's fix that in another PR.

It doesn't look like the Client, ClientEnvironment or ClientMap has a lock

I actually like that. All access to map/mapblock is cleanly from the main thread (except the ones we found). That is easy to reason about. I could revive #16282, but (a) it is currently unnecessary, and (b) causes a quite significant slowdown and client stutter.

[sfan5]

Do you mean this as a general foot gun, or specifically if somebody later changes how Mono-Blocks work on the client?

(the latter)

Locking the client during the copy would effectively be like #16282

It wouldn't. Because when copying while enqueueing there's a real chance that we copy multiple times, while copying just before processing only happens once.

I actually like that. All access to map/mapblock is cleanly from the main thread (except the ones we found). That is easy to reason about.

I don't disagree. But the unsafe access we found does pose a bit of a problem, which we shouldn't leave unsolved.

[lhofhansl]

(the latter)

So let me just make sure that tryShrinkNodes has no effect on the client.

The main thread on the client is actually in Game thread. It's not enough to lock Client::step, or even Game::step. Maybe we can figure out which parts to lock in Game::run, and lock those parts that can possibly access map or mapblocks.

In summary, I do not want to fix locking as part of this PR, and I am not even sure we want do that for 5.13.
But I can easily make sure that this PR is not going to make it worse than it already is.

[lhofhansl]

Like so...

I am happy to file a separate PR and see how we can fix the locking on the client.

Because when copying while enqueueing there's a real chance that we copy multiple times

Yes, you are right.

Update: As I think about it... Note that in the last version of #16282 I had only the one block in question updated (not its surrounding blocks, because that is actually not needed). So there was a full copy when the event is first placed and then only replacing the one potentially changed block, and that was still quite slower (client_mesh_chunk was 6 or 8). So it's that one copy that takes time.

[lhofhansl]

I briefly looked through the client code. There are various places in client/clientMap/game that access the map and/or mapblocks. This is not a quick fix. We'd need to carefully look through all the call and lock what is necessary. The only safe thing would be to lock Game::run which obviously would defeat the purpose.

This is just to reinforce the points: (a) on the client this PR ensures that there are no new issues, (b) getting locking right needs its own PR and some thinking - I'd suggest after 5.13, and (c) let's think hard about keeping/having the invariant that only the main thread accesses map/bocks on the client.

@sfan5 I do think that this is good to merge. Access to the MapNode[] is currently safe and now there are safeguards that nobody would accidentally change it.

[lhofhansl]

Gentlemen, what should I do with this? :) @sfan5 @Desour @appgurueu

[sfan5]

Wait until after 5.13. Then wait more until someone has time.

[lhofhansl]

Wait until after 5.13. Then wait more until someone has time.

I'd like to point out that this PR is now guaranteed to cause no new issues. On the client this is not used, and on the server we do proper locking.
Any locking issues on the client are existing and unaffected by this PR, we can tackle those in a different PR - if we want to.

I am not planning any further changes in this one.

[lhofhansl]

@sfan5 Another safety check: Never compress/shrink the node array when a block is referenced.

[farooqkz]

When is this going to get merged?

[sfan5]

The answer is always the same: It will be when another coredev has reviewed (and ideally tested) the PR himself/herself or someone else who they are confident in has done so.

[lhofhansl]

@sfan5 @appgurueu Any chance to do a review? (I addressed the risk of client race conditions.)

[lhofhansl]

Is there anything you want me to do (other than wait)?

[Desour]

Tested with ubsan, asan, and leaksan (all at once), found nothing.

The thread safety stuff (i.e. why monoblocks are disabled) needs code comments. One shouldn't have to read lengthy PR descriptions to understand code.

(Edit: Also tested with tsan now. Found nothing new as well. But I didn't do much more than fly around in a map to trigger emerge, so I didn't cover all code paths by far.)

[lhofhansl]

Thanks @Desour , all good comments. I'll post an update later today or tomorrow.

[lhofhansl]

Then please also use the same param name in the .cpp file.

Whoops I had that, but somehow undid it before I pushed... Weird. Anyway, done now.

[Desour]

Thanks!

[farooqkz]

Finally merged! Yay!

[sfan5]

Regarding the client safety stuff: by chance I stumbled upon #13124, which is the PR that introduced this thread unsafety (to achieve more performance)