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

Replace clientmap's MeshBufListList with a hashmap #14184

Merged
merged 6 commits into from Jan 3, 2024

Conversation

Desour
Copy link
Member

@Desour Desour commented Dec 29, 2023

  • I once found out during profiling that MeshBufListList::add is a bottleneck.
    (I sadly can't reproduce the profiling results right now, because I can't get hotspot to work again.)
  • MeshBufListList::add has O(n) time where n is the number of unique materials in mapblocks in the scene. And it is called for each meshbuffer.
  • This PR replaces it with a hashmap (see MeshBufListMaps).
  • Can result in a drawtime reduction of 24%.

To do

This PR is a Ready for Review.
TODO: Where should I put PtrHash?

How to test

  • Create a singlenode world with this mapgen mod: https://codeberg.org/Desour/test_mapblock_drawcalls
  • Look towards the red-blue corner, with viewing range 200.
  • Rejoin and wait until drawtime stabilizes. Do the same with the other branch.
  • Results: master: fps 8 Hz, drawtime 112 ms; PR: fps 10 Hz, drawtime 85 ms (76%)
  • Note: Real world results are probably less fruitful. But the fruit is hanging low.
  • Edit: Also tried the spawn of a server: master: fps 19 Hz, drawtime 45 ms, PR 20 fps, 41 ms (91%)

@sfan5
Copy link
Member

sfan5 commented Dec 29, 2023

You could directly write a benchmark similar to your mod in C++ and put it here.
The code would probably need a bunch of adjustments before that however (see #13616).

src/client/clientmap.cpp Outdated Show resolved Hide resolved
src/client/clientmap.cpp Show resolved Hide resolved
@Desour
Copy link
Member Author

Desour commented Dec 29, 2023

You could directly write a benchmark similar to your mod in C++ and put it here.

Idk how to write an automatic benchmark for client rendering performance though.

@sfan5
Copy link
Member

sfan5 commented Dec 29, 2023

Idk how to write an automatic benchmark for client rendering performance though.

Oh, you're right. I didn't see that the code that uses this data structure is so closely linked to actual drawing.

@lhofhansl
Copy link
Contributor

I have not seen this in any profiling I did. But I believe you. I will try this today!

@Desour
Copy link
Member Author

Desour commented Dec 29, 2023

I have not seen this in any profiling I did.

Note that this is not a bottleneck if the scene is simple, for example a fresh mapgen'ed mtg world. You need a scene with many different materials, aka many different node tiles / different textures, for example a busy spawn of a server.
I've created the testing mod specifically for such scenes. And my profiling was done with that mod, for reproducibility.

@lhofhansl
Copy link
Contributor

Tested in an open scene, some trees, many rocks, water, sand, etc. Did not see any measurable difference. It also did not make it worse. :)

@Desour
Copy link
Member Author

Desour commented Dec 30, 2023

Ah, good to hear that the happy little trees did not get slower. c:

I've now added some benchmarks I got from a scene of a server spawn, where I got a 9% drawtime reduction. This is not easily reproducible for you though.

I've also got hotspot to work again (thanks to its appimage distribution).

Here are some flamegraphs that I got with my testing mod:

master:
screenshot_master

PR:
screenshot_pr

You might also notice that the shader constant setter callbacks are another bottleneck. We can probably optimize here. Time of day is not different per mapblock, for example.

@lhofhansl
Copy link
Contributor

I definitely saw the shader constant setter in profiling before.

@Desour
Copy link
Member Author

Desour commented Jan 2, 2024

Removed PtrHash, see #14184 (comment).

PR is ready now.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works well. No issues found so far. Thank you.

@sfan5 sfan5 merged commit 3eab5e9 into minetest:master Jan 3, 2024
13 checks passed
@Desour Desour deleted the meshbuflist_hashmap branch January 3, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants