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

Allow unsafe access into MapSector::m_blocks #14232

Merged
merged 4 commits into from Jan 10, 2024

Conversation

lhofhansl
Copy link
Contributor

Over the weekend I realized that with many blocks loaded, sometime just enumerating all these blocks takes time.
As you fly around in a map and have a large viewing_range set, it is not uncommon to have 100.000 or more blocks loaded on the client.

I so made this simple change, that just allows internal access to a sector's blocks.

  • Goal of the PR
    Reduce the cost enumerating blocks in ClientMap.

  • How does the PR work?
    It allows limited access to the map stored inside MapSector, without making a copy.

  • Does it resolve any reported issue?

  • Does this relate to a goal in the roadmap?

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

Better client performance.
With 60k blocks. touchMapBlocks goes from taking 11.5ms to 9ms, and updateDrawListShadow from 8.5ms to 4.5ms.
With more blocks (especially when loaded, but outside of the current viewing_range) this would be even worse.
This should also help with many allocations, as we copy the MapBlock pointers into new/growing vectors.

To do

This PR is Ready for Review.

How to test

Join any world. Set client_mapblock_limit to 100.000 or more. Fly around until many blocks are loaded. Look at the debug view (F6) and note the time spent in:

  • updateDrawlist (if client_mesh_chunk < 4)
  • touchMapBlocks (if client_mesh_chunk >= 4)
  • updateDrawlistShadow (if shadows are enabled)

@wsor4035 wsor4035 added the @ Engine Core What happens inside the very engine label Jan 8, 2024
@lhofhansl
Copy link
Contributor Author

lhofhansl commented Jan 9, 2024

@sfan5 How about this? Removed non-const access to the new method and changed all callers to use const MapSectors.

Edit: Just saw your last message now, so I assume you are good with that :)

@lhofhansl
Copy link
Contributor Author

lhofhansl commented Jan 9, 2024

Perhaps I should allow the old getBlocks from a const object as well.
Not sure about getBlockNoCreateNoEx as it modifies the internal cache, I guess it is still semantically read-only.

Both are not needed for this change, but be good for consistency.
And I guess I could remove the Unsafe part, since now it is safe. :)

src/mapsector.h Outdated Show resolved Hide resolved
@sfan5
Copy link
Member

sfan5 commented Jan 9, 2024

Not sure about getBlockNoCreateNoEx as it modifies the internal cache, I guess it is still semantically read-only.

The correct way to do that is to mark the cache variables are mutable so they can still be used on a const object.
But let's not over-complicate this PR.

Co-authored-by: sfan5 <sfan5@live.de>
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lhofhansl lhofhansl merged commit 4bf9570 into minetest:master Jan 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants