Remove spurious memory allocations in TextureCacheBase::SerializeTexture and DeserializeTexture #9641
Conversation
|
Savestate performance is always appreciated. You may have to squash that last commit unless you want it to be a separate commit for some reason. |
|
Yes. It's the kind of change that won't be noticed by most users but would be greatly appreciated by a small subset of users. If you have more optimizations you can make, I'm interested in seeing those as well. All three commits should be squashed into one, not just the last two. I agree that adding this kind of |
lynlevenick
force-pushed
the
ash/texture-cache-opt
branch
from
c4f8e97
to
2d8df0c
Compare
|
Addressed comments and squashed the commits together. I'll open up some other changes as separate PRs once I've pulled them out of my branch. edit: My other changes at present are mildly invasive - avoiding some GPU flushes at a memory tradeoff, adding a threadpool to saturate memory bandwidth - which might hit resource limits or perform worse on some platforms/configurations, so I'm not sure Dolphin would want them. edit 2: Cleaned up the comment again (it was getting confused with my other branch, apologies). |
texture serialization and deserialization used to involve many memory allocations and deallocations, along with many copies to and from those allocations. avoid those by reserving a memory region inside the output and writing there directly, skipping the allocation and copy to an intermediate buffer entirely.
lynlevenick
force-pushed
the
ash/texture-cache-opt
branch
from
2d8df0c
to
43ceba4
Compare
I'm doing a project with Dolphin which wants to save state incredibly frequently (~every frame). Reallocating
texture_datainTextureCacheBase::SerializeTexturetakes up a non-negligible amount of the serialization time in my environment. For my own project I might be able to implement a check to see if textures have changed in the last frame and avoid re-serializing them, but this is general enough to save everybody some fractions of a second when they save state - maybe TASers would appreciate it?This PR fixes the issue by adding a method to
PointerWrapwhich sets aside some space for the caller to read/write/verify, then using it to set aside one large chunk of space all at once in the texture cache. This should not change the save-state format.Additionally, the new method is used when deserializing textures as well to avoid a single memory allocation/copy/deallocation, but that's of much less concern and could be removed pretty easily.
I'm not a huge fan of the method I went with here - all of the paths that hit
DoExternalneed to handle themodeof thePointerWrap, but at present there's no enforcement of that. I've personally verified that only read and write operations take place to the pointers, but that's no guard against future changes so it's pretty reasonable to sayDoExternalis going too far/breaking abstraction. If so, let me know: I may be able to change this to refactorSerializeTextureto do one allocation instead of worst-case layers * levels copies/reallocations.If save state performance is important, I've got a few other lower-impact changes which I could also try to contribute if desired.