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

Optimize shared texture creations #98760

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ze2j
Copy link
Contributor

@ze2j ze2j commented Nov 2, 2024

Fix #98733

This PR removes an unnecessary copy of Texture::slice_trackers, as it is always cleared afterward.

Build: scons platform=linuxbsd optimize=speed_trace debug_symbols=yes
Profiler: Tracy
Test project: see MRP in #98733

Results with master branch:
master

Results with this PR:
pr

This commit removes an unnecessary copy of `Texture::slice_trackers`,
as it is always cleared afterward.
Comment on lines +906 to 907
Texture texture = src_texture->duplicate_as_shared_texture();
texture.shared_fallback = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

I guess duplicate_as_shared_resource should also avoid copying shared_fallback so we don't have to manually set it to nullptr right after doing the copy

@@ -1055,7 +1053,7 @@ RID RenderingDevice::texture_create_shared_from_slice(const TextureView &p_view,
slice_layers = 6;
}

Texture texture = *src_texture;
Texture texture = src_texture->duplicate_as_shared_texture();
Copy link
Member

Choose a reason for hiding this comment

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

I find it a bit surprising that we don't call texture.slice_trackers.clear(); here.

CC @DarioSamo who added the call in texture_create_shared(). Do you think this was missed by mistake? Or is there a reason to maintain the slice_tracker when creating a shared texture from a slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clear is made in _texture_make_mutable

@DarioSamo
Copy link
Contributor

I think slice_trackers should be turned into a pointer instead and allocate it on demand only if a shared texture is created. The HashMap currently occupies 48 bytes per texture and largely goes unused in the vast majority of cases. Then when the copy is made, the simple solution is to just set it to a nullptr. That way there'll be no cost from copying the contents as is and we get a reduced memory footprint overall, for the small extra cost of an indirection.

I'm not a big fan of the proposed solution as it adds another function to maintain and keep track of when the copy is made.

@ze2j
Copy link
Contributor Author

ze2j commented Nov 4, 2024

I agree and I am not a big fan of my solution too : ) I am going to rework my PR with your solution, it's a nice idea.

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.

RD::texture_create_shared_from_slice become very slow when used extensively on a Texture2DArrayRD
4 participants