ServerMap: serialize map blocks without holding m_db.mutex

[johnnyjoy]

Map block serialization and compression used to run while holding m_db.mutex, which also guards loads and other DB access. This change builds each block’s blob first, then takes the mutex only for the actual MapDatabase write. Periodic ServerMap::save() batches dirty blocks into a vector, then uses one lock scope for beginSave → writes → endSave. beginSave() and endSave() follow the usual pattern again: each takes m_db.mutex and calls the database’s beginSave() / endSave() (including for nested callers such as Map::timerUpdate).

Goal of the PR

Reduce the time spent with m_db.mutex held during saves so that loads and other users of the map DB wait less, especially on busy servers or with slow storage.

How does the PR work?

• serializeMapBlock(MapBlock *, compression_level) — returns std::string (version byte + serialized data), no mutex.
• saveSerializedMapBlock(MapBlock *, MapDatabase *, std::string_view) — calls db->saveBlock(pos, data) and clears modified flags on success.
• ServerMap::save() — collects (MapBlock*, blob) for blocks that need saving, then under one MutexAutoLock: beginSave() on DB, loop saveSerializedMapBlock, endSave().
• beginSave() / endSave() — each takes m_db.mutex and delegates to m_db.dbase->beginSave() / endSave() (same overall shape as before this work, without extra ServerMap batch state).
• saveBlock(MapBlock *) — serialize blob outside the lock, then lock and write via saveSerializedMapBlock.

Does it resolve any reported issues?

No separate GitHub issue is required; it addresses the old FIXME: serialization happens under mutex concern in servermap.cpp by moving serialization out of that critical section; a separate zero-copy FIXME remains on the serialization buffer path. It continues the performance thread from PR #15151.

Does this relate to a goal in the roadmap?

Not explicitly tied to a numbered item there. It is a server performance/responsiveness improvement (shorter critical section on the map DB mutex).

If not a bug fix, why is this PR needed? What use cases does it solve?

Not a correctness bug fix; it is an optimization. Use cases: dedicated servers with heavy map churn, large periodic saves, or slow disks where serialization under the lock stretched contention with block loads and similar work.

AI / LLM disclosure

Cursor AI was used in editing.

Files changed

File	Role
src/servermap.cpp	Save path, serializeMapBlock, saveSerializedMapBlock, beginSave/endSave, saveBlock
src/servermap.h	Includes, static helper declarations

Note

Please reject if you don't have time to review or don't want to deal with it. I am using this code locally, so I'm sharing it. I am currently moving away from this method for my personal use while experimenting locally with a 64-mutex stripe arrangement, which will likely become my norm.

[lhofhansl]

Generally I like this idea.
(IMHO, the bad lock is not the DB lock, but the env lock. Blocks are both serialized and de-serialized under the env lock... But this helps as well.)

I am currently moving away from this method for my personal use while experimenting locally with a 64-mutex stripe arrangement

What's this new implementation?
Also do you have any numbers before/after your PR?

[johnnyjoy]

lhofhansl,

What I’m doing locally is related to the current PR, but aimed at a slightly different layer of the same problem.

The PR that moves serialization outside the mutex is the obvious first win. Holding the DB lock while doing CPU work (compressing/encoding the block) just extends contention for no reason. Getting that work out from under the lock shortens the critical section and helps immediately, so I think that’s the right direction regardless of anything else.

What I’ve been experimenting with on top of that is reducing the scope of the lock itself. Right now, even with shorter hold times, all mapblock DB access still funnels through a single mutex. So unrelated loads/saves still end up taking turns. The stripe approach just spreads that out: a small fixed set of mutexes (I’ve been using 64), and each block position hashes to one of them. It’s not perfect isolation, but in practice, it lets different areas of the map proceed in parallel most of the time without the overhead of a mutex per block.

So to me, these aren’t competing ideas; they stack. First, shrink how long the lock is held (serialize outside), then reduce how many things are forced to share the same lock in the first place (striping).

As for numbers, nothing solid enough to present yet. This is still local experimentation, not something I’ve run through a proper benchmark harness. In earlier work with a PostgreSQL + Memcached setup (years back on the FREE PIZZA server), I saw very high cache hit rates, 95% at times, and that’s where this kind of thing starts to matter; once your backend is fast, the in-process locking becomes the bottleneck. The goal here is to remove that bottleneck so that concurrency gains from the backend or caching can actually be seen.

I will look into m_env_mutex and see if I can coax form friend gains from it. I run a server at my work for many people to test on, but I might open a public server again sometime soon.

If only I had more spare time.

[johnnyjoy]

"I will look into m_env_mutex and see if I can coax form friend gains from it. I run a server at my work for many people to test on, but I might open a public server again sometime soon."

I meant "coax some gains from it," but Grammarly had other plans. :)

[sfan5]

The stripe approach just spreads that out: a small fixed set of mutexes (I’ve been using 64), and each block position hashes to one of them. It’s not perfect isolation, but in practice, it lets different areas of the map proceed in parallel most of the time without the overhead of a mutex per block.

So do you have 64 database connections then? Or how is thread safety assured?

[sfan5]

For future reference: the contention of the db mutex is between the server thread, which (mainly) saves blocks and the emerge thread, which loads blocks.

[johnnyjoy]

The stripe approach just spreads that out: a small fixed set of mutexes (I’ve been using 64), and each block position hashes to one of them. It’s not perfect isolation, but in practice, it lets different areas of the map proceed in parallel most of the time without the overhead of a mutex per block.

So do you have 64 database connections then? Or how is thread safety assured?

No, not 64 PostgreSQL connections, but I thought about it. The “64” in this design is 64 in-process mutex stripes on the C++ MapDatabaseAccessor, not 64 PGconn objects.

What the 64 is MapDatabaseAccessor has 64 std::mutex entries (STRIPE_COUNT = 64) used to pick a stripe from a block position (hash). That is server-side locking around how the map uses the MapDatabase. It does not allocate 64 connections to Postgres. It would take some work to create the architecture for that.

https://github.com/johnnyjoy/luanti-rollback-refactor/tree/mutex-experimental

[sfan5]

again, why remove these comments?

[sfan5]

or this