We now pool ContentManager scratch buffers#5921
Conversation
|
I agree 100%. Here is a bufferPool I'am testing and works exactly like that. The only danger is if the user hit it with dozens or hundreds of threads at once. Should we use a Semaphore or something there? |
|
You mean to allow a maximum of N threads to use a buffer from the pool at once? We could do that, it would certainly make this more foolproof :) |
|
yes, it's an extreme case of course. Edit |
|
I prefer the second option! I'll close this since you already implemented the better pool. |
|
I kind of stuck on this issue and then forgot about it. |
|
no, I'am ok with it. |
|
|
||
| internal void ReturnScratchBuffer(byte[] buffer) | ||
| { | ||
| _scratchBufferPool.Return(buffer); |
There was a problem hiding this comment.
How about just making _scratchBufferPool internal and let people call it directly? Just to avoid these redirect methods. Just make ByteBufferPool ctor take an optional minimum size.
There was a problem hiding this comment.
I am not sure if it's best to modify ByteBufferPool or use a second pool and let the two take different roads for contentManager & soundEffects. In every case I prefer having a layer that allows us to be more flexible with the internal implementation. Maybe having an IBufferPool with Get()/Return() or something like that.
|
@Jjagg |
|
@nkast We can modify BBP to have a maximum number of items in the pool and a minimal buffer size. Maximum pool size for ContentManager can be set to Environment.ProcessorCount. That way we only need one implementation. |
|
Anything left to do? This seems like a good solution to me :) |
|
I modified the ByteBufferPool to remove a buffer when it does not have a large enough buffer for a @nkast, @tomspilman Sounds good? |
Is that desirable for SoundEffectInstances ? Other than that those changes will work very well for the ContentManager. |
|
I think it is. We use the ByteBufferPool for DynamicSoundEffect instances. The buffers there are not immediately returned like the ScratchBufferPool buffers, but when the DSFX is stopped. So the pool will grow to the maximum number of DynamicSoundEffect instances playing at the same time and no larger. |
|
Bump! IMO this is good to go like this :) |
|
Bumping because I think someone might've ran into the same issue as #5341 (which this fixes): http://community.monogame.net/t/contentmanger-not-unloading-properly/10550 |
ContentManager uses these for its ScratchBufferPool.
|
@harry-cpp This is good to go! |
As discussed in #5341. (edit for autoclose: fixes #5341)
I don't think this is right as is. Consider the case where a user loads incrementally larger assets. For each asset a new scratch buffer will be created and returned to the pool, staying in memory until game exit.
Maybe it would be better to use a slightly modified version of ByteBufferPool that deletes the smallest buffer from the pool whenever Get is called and there are no buffers that are large enough. That way the pool size won't grow beyond the number of threads that use a buffer from it simultaneously (most often 1).
cc @nkast