Optimize TileMapLayerEditor

[Nazarwadim]

Before:

before.mp4

After:

after.mp4

[clayjohn]

This is a very nice improvement for the pathological case in #105683

I don't think it is enough to call the issue "solved" but it moves the bottleneck significantly.

Before:

After:

[clayjohn]

@Nazarwadim Can you please rebase on master to prepare this for merging?

Note that editor/plugins/tiles/tile_data_editors.cpp was renamed to ‎editor/scene/2d/tiles/tile_data_editors.cpp.

[groud]

Does this increase performance compared to the previous code?:

TileSetSource *source = nullptr;
if (tile_set->has_source(tile_source_id)) {
	source = *tile_set->get_source(tile_source_id);
}

If not, I think it's a bit pointless to create a function just for that. And if it does improve performance, I think it should be fixed in many other places, not just here.

[clayjohn]

We are slowly rooting out the "has/get" pattern throughout the codebase. It needlessly duplicates all the expensive parts of using a Hashmap. Literally doing the exact same expensive operation twice.

You can't really compare the old code to the new one here, since, after fixing the other issues has_source() becomes more of a bottleneck than it was in the original code.

What happens exactly is:
Before: the old code is so slow that you only get a few frames per second. So has_source() / get_source() appears insignificant
After: More frames are processed per second which multiplies the cost of has_source() / get_source() so cutting the combined cost in half ends up being worthwhile

[groud]

I mean, I am not against the idea of getting rid of the has_something / get_something pattern. It's just that this is used EVERYWHERE in the TileMap / TileSet and the respective editor code. If we go that route, I'd rather have it fixed everywhere rather than this single instance.

And this should be done for:

• sources
• atlas coords
• alternative tiles IDs
  not sources only.

We can merge it as is if we consider this performance culprit a blocker. I just that I believe this specific optimization should be done consistently, for both performance and maintainability.

[Shadows-of-Fire]

That optimization should be done consistently but it isn't a reason to stall this PR. This PR has significant improvements for anyone working with TileMapLayer(s) of non-trivial sizes.

[groud]

The main problem in this code is indeed the use of get_used_cells. I fixed a few in #107559 but missed that one I guess. I think that specific change is good, but other changes aren't really needed I think.

[clayjohn]

The main problem in this code is indeed the use of get_used_cells. I fixed a few in #107559 but missed that one I guess. I think that specific change is good, but other changes aren't really needed I think.

Take a look at the profiling results I posted above 32% of the cost of the function is from calling has_tile(). This PR significantly reduces the usage of has_tile(). get_used_cells() was only responsible for 14% of the cost of the function. Literally less than half the benefit of this PR is from removing get_used_cells()

[clayjohn]

Here is a clearer capture of the profiles. In this case, both are a sampler of 10 seconds.

Master

3.25 seconds out of 10 spent on has_tile()

This PR

Reduced to 750 ms! This is also misleading, because FPS is way higher now, so the 10 seconds has more calls to draw_overlay()

[groud]

Take a look at the profiling results I posted above 32% of the cost of the function is from calling has_tile(). This PR significantly reduces the usage of has_tile(). get_used_cells() was only responsible for 14% of the cost of the function. Literally less than half the benefit of this PR is from removing get_used_cells()

I mean, the PR does not remove the call to has_tile. Not sure how that would improve.

[clayjohn]

Take a look at the profiling results I posted above 32% of the cost of the function is from calling has_tile(). This PR significantly reduces the usage of has_tile(). get_used_cells() was only responsible for 14% of the cost of the function. Literally less than half the benefit of this PR is from removing get_used_cells()

I mean, the PR does not remove the call to has_tile. Not sure how that would improve.

I can think of a couple reasons, but what I think is most likely is the source hashmap is more likely to reside in cache now. Previously we messed up the cache by iterating over the entire tile_map_layer_data and then indexing into it multiple times. The new code is more likely to leave the source hashmap in cache between iterations which would remove most of the cost from has_tile()

[groud]

I can think of a couple reasons, but what I think is most likely is the source hashmap is more likely to reside in cache now. Previously we messed up the cache by iterating over the entire tile_map_layer_data and then indexing into it multiple times. The new code is more likely to leave the source hashmap in cache between iterations which would remove most of the cost from has_tile()

Hmm I don't know. I'll make a branch just removing get_used_cells, so we can compare between this PR and my branch. Because if it's a cache issue, maybe dealing with another instantiated hashmap messes things too.

[groud]

Ok, I made this branch: https://github.com/groud/godot/tree/remove_get_used_cells @clayjohn if you wanna try it out :)

[clayjohn]

@groud Here you go!

I highly recommend getting comfortable with a profiler yourself. It helps you build intuition for where changes will actually benefit performance. Importantly, it means you no longer have to guess.

[groud]

I highly recommend getting comfortable with a profiler yourself. It helps you build intuition for where changes will actually benefit performance. Importantly, it means you no longer have to guess.

Sorry, since I needed to test three setup (as we don't have the same machine) I was a bit a lazy on that one. But you're right :p

But alright. I guess the change is quite efficient then. If we think we don't need to fix the issue everywhere right now, the PR is fine (beside the small double-check I mentioned)

[Nazarwadim]

I fixed the bottleneck has_alternative_tile by creating faster has_tile_with_alternative that doesn't make unnecessary hashmap calls and replacing HashMap with AHashMap for alternatives.

[Shadows-of-Fire]

@clayjohn @groud Is there anything that needs to happen for this change to be merged? The perf issues with this editor are a huge problem for large tile maps.

[clayjohn]

If ground has no objections, I think it could be merged after a rebase

[groud]

If ground has no objections, I think it could be merged after a rebase

Yeah, I don't think I like it's that much, as it only fixes the the issue in one specific instance. But well, it's probably the most annoying one, so let's go for it. Once rebased I think it can be merged.

[Shadows-of-Fire]

I'm not sure if @Nazarwadim is available to do the rebasing (they have had little activity over the past few months, and this PR is 1.5 years old).

I've gone ahead and rebased [and squashed] it here https://github.com/Shadows-of-Fire/godot-engine/tree/opt-tilemaplayer-editor

The actual rebase was a trivial indentation change in the last hunk of tile_map_layer_editor.cpp due to the check introduced in c3fdc85.

I can either make a PR from my rebased branch that points to this one, or if this maintainers are able to edit Nazar's source branch you can push the rebase there and we can continue on this PR.

[Nazarwadim]

@Shadows-of-Fire I willl rebase this soon

[Shadows-of-Fire]

@Nazarwadim Do you have any idea of when you'll be available for this? If you don't have time I can just open another PR from my rebased branch. I'm hoping to avoid the need for more rebases and also avoid a slip from the next release.

[Shadows-of-Fire]

@groud @clayjohn Rebase is in, looks like the AI is back on either of you to do a final sanity pass.

[Shadows-of-Fire]

@groud @clayjohn Poking again. Would like to see this for 4.7