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

Cache MapBlock data in the mesh generator thread for less stutter #5595

Merged
merged 2 commits into from Apr 17, 2017

Conversation

Projects
5 participants
@celeron55
Copy link
Member

celeron55 commented Apr 15, 2017

Adds a MapBlock cache in MeshUpdateThread's MeshUpdateQueue. This way only changed MapBlocks need to be copied from the main thread, plus ones that have timed out from the cache.

Neighboring MapBlocks are often received and updated in close succession to each other, which leads to huge amounts of MapBlocks being fetched from the cache.

This removes the MeshMakeData::fill stutter that is seen on the client when large amounts of received MapBlocks are being handled. There are multiple other unknown sources of stutter left, which can make noticing the difference difficult.

A version with some extra instrumentation can be found at https://github.com/celeron55/minetest/tree/mesh_update_block_cache.

Relates slightly to #5239.

b->copyTo(m_vmanip);
}
for(u16 i=0; i<26; i++)
{

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 15, 2017

Member

wrong style

for(std::vector<QueuedMeshUpdate*>::iterator
i = m_queue.begin();
i != m_queue.end(); ++i)
{

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 15, 2017

Member

please fix the style

@@ -26,6 +26,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#include "../threading/mutex_auto_lock.h"
#include "porting.h"
#include "log.h"
#include "container.h" // MutexedQueue

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 15, 2017

Member

no container here, the container.h include should be in another place

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

Do we really intend to postpone header inclusions for classes required by templated classes like that? Well ok...

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

I mean, if you ctrl+f that file, MutexedQueue is required there. It's just that the compiler doesn't care until you actually try to use the classes in util/thread.h as they require template parameters.

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

Well whatever, less includes inside the most common headers is faster to compile so I'll go with that.

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

That just means that even the order of inclusion is important; I need to include "util/container.h" in mesh_generator_thread.h before "util/thread.h", otherwise there's a compiler error.

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

Wait actually, it's not even what I initially said!

The headers are just purely broken and it really needs to be there. I'll put it in a separate commit.

v3s16 dp;
for (dp.X = -1; dp.X <= 1; dp.X++) {
for (dp.Y = -1; dp.Y <= 1; dp.Y++) {
for (dp.Z = -1; dp.Z <= 1; dp.Z++) {

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Apr 15, 2017

Member

Indents can be reduced here since we allow nested loops to be on the same level.

@paramat paramat added the Performance label Apr 15, 2017

@celeron55 celeron55 force-pushed the celeron55:mesh_update_block_cache_pr branch 2 times, most recently from 872f046 to 8364d43 Apr 15, 2017

@@ -154,6 +154,11 @@ class MapBlock /*: public NodeContainer*/
raiseModified(MOD_STATE_WRITE_NEEDED, MOD_REASON_REALLOCATE);
}

MapNode* getData()

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 15, 2017

Member

const MapNode* ?

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 15, 2017

Author Member

Can't do that; there are so many non-const interfaces everywhere.

I tried it.

@paramat paramat added this to the 0.4.16 milestone Apr 15, 2017

@celeron55 celeron55 force-pushed the celeron55:mesh_update_block_cache_pr branch from 737e623 to 29d60df Apr 15, 2017

@celeron55

This comment has been minimized.

Copy link
Member Author

celeron55 commented Apr 16, 2017

Meshgen block cache size vs. hit %

Test conditions:
Flying fast in a previously generated Voxelgarden f1c4b8630a world, doing a
single loop around a mountain, taking about 23 seconds, at fast speed.

i7-4800MQ, 16 GB RAM

  max_simultaneous_block_sends_server_total = 10
  max_simultaneous_block_sends_per_client = 10
  max_block_send_distance = 20
  max_block_generate_distance = 20
  block_send_optimize_distance = 15
  viewing_range = 200
  fast_move = true
  free_move = true
  mesh_generation_interval = 0
  meshgen_block_cache_size = X

As the cache limit only dynamically controls the maximum age of blocks in the
cache, the actual cache size is dynamic and varies based on usage patterns.

1000 MB: 94% (actual size ~75 MB)
 100 MB: 94% (actual size ~48 MB)
  50 MB: 94% (actual size ~34 MB)
  20 MB: 93% (actual size ~15 MB)
  10 MB: 87% (actual size ~7.5 MB)
   5 MB: 82% (actual size ~3.8 MB)
   3 MB: 77%
   2 MB: 73% (actual size ~2.0 MB)
   1 MB: 72%

I set the default to meshgen_block_cache_size = 20. The hit % is visible in the profiler as "MeshUpdateQueue MapBlock cache hit %". Some measurements on low-end systems could be useful.

@celeron55 celeron55 force-pushed the celeron55:mesh_update_block_cache_pr branch from 12211bd to 5f599d4 Apr 16, 2017

@celeron55

This comment has been minimized.

Copy link
Member Author

celeron55 commented Apr 16, 2017

On the VE-S server, 96 hit% can be seen when standing still, which indicates all the neighbors (3*3*3) of each MapBlock being updated can be found in the cache, which is the ideal scenario for this system, given that each mesh update requires 3*3*3 MapBlocks, all of which except the middle one can be fetched from the cache. (1 - 1./(3*3*3) = 0.963)

outdated

/*
Mark the block as urgent if requested
*/
if(urgent)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 16, 2017

Member

space

This comment has been minimized.

Copy link
@celeron55

celeron55 Apr 16, 2017

Author Member

It's all old code moved as-is from client.cpp. Should I reformat it all?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 16, 2017

Member

yes ofc

@celeron55 celeron55 force-pushed the celeron55:mesh_update_block_cache_pr branch from 5f599d4 to 5ff54e0 Apr 16, 2017

@celeron55

This comment has been minimized.

Copy link
Member Author

celeron55 commented Apr 16, 2017

I added a commit right after the mesh_generator_thread.cpp commit that fixes the code style of the old code. I guess this is a bit messy for following the development of the PR but the commits look nice this way.

@celeron55

This comment has been minimized.

Copy link
Member Author

celeron55 commented Apr 17, 2017

When playing using Android view ranges like 25, the cache tends to stay at under 5 MB and at 94 hit%.

Personally I think this is mergeable now, anyone 👍?

@nerzhul
Copy link
Member

nerzhul left a comment

Okay for me

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 17, 2017

I support the concept.

@celeron55

This comment has been minimized.

Copy link
Member Author

celeron55 commented Apr 17, 2017

I'll be merging this as two commits, one for the util/thread.h include fix and one for all the rest of it.

celeron55 added some commits Apr 15, 2017

MeshUpdateQueue: Add a MapBlock cache that minimizes the amount of Ma…
…pBlock copying done in the main thread

Cache size is configurable by the meshgen_block_cache_size (default 20 MB).

New profiler stats:
- MeshUpdateQueue MapBlock cache hit %
- MeshUpdateQueue MapBlock cache size kB

Removes one type of stutter that was seen on the client when received MapBlocks
were being handled. (the "MeshMakeData::fill" stutter)

Kind of related to at least #5239

Originally preceded by these commits, now includes them:
- Move the mesh generator thread into src/mesh_generator_thread.{cpp,h}
- mesh_generator_thread.cpp: Update code style
- MeshUpdateThread: Modify interface to house a different implementation: Actual functionality will be changed by next commits.
- MeshMakeData: Add fillBlockData() interface (so that caller can fill in stuff from eg. a MapBlock cache)

@celeron55 celeron55 force-pushed the celeron55:mesh_update_block_cache_pr branch from 5ff54e0 to 160c551 Apr 17, 2017

@celeron55 celeron55 merged commit 04cc9de into minetest:master Apr 17, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@nerzhul nerzhul added this to Done in Minetest 0.4.16 Apr 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.