Time  Nick      Message
18:29 lhofhansl Hey... Looking into shared locking for mapblock. std::shared_lock is only available in c++14 onwards, and we limit to c++11 (at least on Linux)
18:30 lhofhansl Any advice?
18:31 sfan5     don't you need a read/write lock anyway?
18:34 celeron55 what are you trying to do?
18:35 celeron55 originally i wrote mapblocks to have mutexes and it was awfully slow, there's so much random access to them all over the place
18:36 celeron55 probably don't want to repeat the same mistake
18:38 lhofhansl Hmm... Trying to break the lock between mapsaving, map generation, and client handling.
18:38 lhofhansl All of these are under one giant lock - the env lock.
18:40 lhofhansl celeron55: Was is slow because of contested locks, or was it slow due to the memory barrier locks need to take even when not contested?
18:41 lhofhansl Perhaps it's better to lock at the sector level...?
18:41 celeron55 well the same code ran fast when i removed the locks
18:41 celeron55 you can't do any locking that causes a lock/unlock at each node access
18:43 lhofhansl Probably memory barriers them, which especially slow down multiple threads on multiple cores.
18:44 celeron55 accessing a node currently usually causes just two pointer references and then indexing the node array in the mapblock
18:44 celeron55 it's a way faster code path than you might think
18:45 celeron55 well ok, also comparing the position of the cached sector and mapblock, but that's nothing also
18:45 celeron55 the last used sector and last used mapblock is cached
18:45 lhofhansl I totally believe that. I've seen bad locking slowing down services to a crawl even without any lock contention. (in real life I work on large distributed databases)
18:46 lhofhansl I saw that part. When I tried to remove MapSector completely I realized how important that cache was :)
18:46 celeron55 then when the sector or mapblock change, they have to be looked up from the std::maps
18:48 celeron55 if you add a mutex, you have to implement some kind of node area locking that will then have to be used from every piece of code that's iterating through map content
18:48 celeron55 (or some other lock)
18:48 lhofhansl So what can be done? What's I'm seeing is that the server is not utilizing it's H/W well. When a block is compressed and saved, no other thread can do any work on the map.
18:48 celeron55 so that each iteration doesn't cause extra operations
18:48 lhofhansl That's what I was thinking. Keep the outer lock, but it cover a subset of the map.
18:48 lhofhansl Do you think a MapSector is too small for that?
18:49 celeron55 what's done currently is that when something is handed to another thread, the data is copied
18:49 celeron55 if copying the mapblock in memory is much faster than compressing and saving it, you could make a copy first
18:49 celeron55 then compress and save it in another thread
18:49 lhofhansl I tried that for MapBlocks, but since they reference other stuff (iGameDef, etc) I still got corrupted data.
18:50 celeron55 everything they reference is basically unmutable, no?
18:50 celeron55 you'll probably have to change some code to not do stupid things if you do that
18:50 lhofhansl Not the id-mapping it seems. I tried the emerge threads to pre-compress the block, and found the node-id mapping is wrong.
18:51 lhofhansl I'll think about this more. Nothing concrete. Operating on immutable data or copies is nice.
18:51 celeron55 well the mapping is created when compressing?
18:52 celeron55 there's no such thing in mapblocks in memory
18:52 lhofhansl Yes, it
18:52 lhofhansl 's derived from somewhere else.
18:53 celeron55 and node ids never change
18:53 celeron55 i think you just created some kind of bug and thought it meant what you were doing was impossible
18:54 lhofhansl I just called serialize on the mapblock and cached that until the block's state was changed. That does not work.
18:54 lhofhansl https://github.com/minetest/minetest/blob/master/src/mapblock.cpp#L390
18:54 lhofhansl Has to do with this.
18:56 lhofhansl But not even trying to lock at the block level is good advice, saved me some time :)
18:58 celeron55 i think collecting a log of node accesses could be useful
18:59 celeron55 just to see the pattern and where the accesses originate from
18:59 lhofhansl I also tried adding a copy constructor to mapblock and then saving a copy of the block. Interestingly I got the same id mismatch. I don't have the exact error message anymore, but it happened when the block was loaded again.
18:59 celeron55 then you can think about if modifying the sources of most accesses is possible to a block or some kind of volume level locking
18:59 celeron55 if it is possible, then you can try doing it
19:00 celeron55 however you never know what mods want to do
19:00 celeron55 so having the current fast access but wide locking works best i think
19:00 lhofhansl Better avoid fine grained locking if possible.
19:01 celeron55 well i don't understand why you would get an id mismatch
19:01 celeron55 or what code would even detect it
19:16 lhofhansl I can post a PR with the code, maybe folks can have a look what I am missing.
19:16 lhofhansl (anyway work calls for now)
22:03 Andrey01  is my long-awaited PR going to be merged eventually? #10363
22:03 ShadowBot https://github.com/minetest/minetest/issues/10363 -- [MainMenu] Add clear button and icon for search one. by Andrey2470T
22:05 Andrey01  this contains actually extremely simple changes and two months (even longer) are required for review...
22:06 specing   Andrey01: two months? Better get a coffee.. :P
22:06 specing   *a barrel of*
22:07 Andrey01  yes, two months. The recent review was fulfilled on 19th September by sfan5
22:08 Andrey01  XDDD