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

New pooled memory types #6821

Merged
merged 3 commits into from
Jun 3, 2024
Merged

New pooled memory types #6821

merged 3 commits into from
Jun 3, 2024

Conversation

jhorv
Copy link
Contributor

@jhorv jhorv commented May 17, 2024

This PR adds 2 new types for using pooled memory:

  • MemoryOwner<T> is a generic class for easy access to pooled memory.
  • SpanOwner<T> is generic value type (readonly ref struct) that serves the same purpose in more constrained scopes, without requiring an additional allocation to manage it.

Both are intended for future optimization and to eventually replace the existing ByteMemoryPool and ByteMemoryPool.ByteMemoryPoolBuffer types in Ryujinx.Common.Memory, which have the following limitations:

  • They only support byte buffers
  • The IMemoryOwner<byte> implementation (the nested ByteMemoryPoolBuffer class) is private and returned as the interface. Usage as the concrete type would be faster.
  • They don't have a value-type version, meaning their use requires allocating a ByteMemoryPoolBuffer instance.

This PR also uses the SpanOwner type to target a memory profiler hotspot, replacing a new array allocation with pooled memory in Ryujinx.Graphics.Vulkan.MultiFenceHolder. The table below shows the measured # of new FenceHolder[] allocations per second of gameplay (actual gameplay, past the title/intro/loading etc.).

Before this PR (a.k.a., # of Allocations Eliminated by this PR)

Game Test Duration Allocations Allocations / second
Metroid Prime Remastered 1 min 2.725 sec 395,500 6,305
TotK 1 min 1.19 sec 769,100 12,569

@github-actions github-actions bot added gpu Related to Ryujinx.Graphics graphics-backend:vulkan Graphical bugs when using the Vulkan API labels May 17, 2024
@ryujinx-mako ryujinx-mako bot requested review from gdkchan, riperiperi and a team May 17, 2024 02:44
@gdkchan
Copy link
Member

gdkchan commented May 17, 2024

There are other custom pool implementations that we currently have, like https://github.com/Ryujinx/Ryujinx/blob/master/src/Ryujinx.Graphics.Vic/Image/BufferPool.cs
Do you think this would be eventually able to replace those?
One think to note is that, at least for this linked buffer pool, the size of the buffers are rather large (a few MB in memory iirc).

@jhorv
Copy link
Contributor Author

jhorv commented May 17, 2024

There are other custom pool implementations that we currently have, like https://github.com/Ryujinx/Ryujinx/blob/master/src/Ryujinx.Graphics.Vic/Image/BufferPool.cs Do you think this would be eventually able to replace those?

They could be replaced, but I think it'd be wise to study them for possible behavior/perf differences. If any of them wrap ArrayPool, those could be drop-in replaced by these new ones without much concern. But any that have a custom algorithm (like BufferPool you mention) may have significantly different perf characteristics that we should probably measure before replacing.

@gdkchan
Copy link
Member

gdkchan commented May 17, 2024

There are other custom pool implementations that we currently have, like https://github.com/Ryujinx/Ryujinx/blob/master/src/Ryujinx.Graphics.Vic/Image/BufferPool.cs Do you think this would be eventually able to replace those?

They could be replaced, but I think it'd be wise to study them for possible behavior/perf differences. If any of them wrap ArrayPool, those could be drop-in replaced by these new ones without much concern. But any that have a custom algorithm (like BufferPool you mention) may have significantly different perf characteristics that we should probably measure before replacing.

I think ArrayPool originally had a pretty low size cap before, low enough that if I tried to rent those buffers using this approach it would not be actually pooled and would just allocate a new array every time. I think around .NET 7 they increased it, so probably should not be a problem anymore. But yea, if its replaced in the future, it will need to be tested carefully. That specific use case is somewhat special since VIC usually allocates a few buffer for the video frames when the decoding starts, and keeps using them until decoding ends. So the lifetime of those buffers is pretty well defined in those cases.

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

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

Looks good, sorry for the late review!

@gdkchan gdkchan merged commit 1ecc8fb into Ryujinx:master Jun 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Related to Ryujinx.Graphics graphics-backend:vulkan Graphical bugs when using the Vulkan API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants