fix: networkscenemanager not releasing buffers from pool #1132
Conversation
Don't used pooled NetworkBuffers for SceneEventData, instead just allocate 1 dedicated NetworkBuffer per SceneEventData (which will yield 2 instances per NetworkSceneManager). Make sure we dispose the scene manager so it will dispose the two SceneEventData instances so they will Dispose their NetworkBuffers.
| @@ -754,7 +754,7 @@ public void Dispose() | |||
| internal SceneEventData(NetworkManager networkManager) | |||
| { | |||
| m_NetworkManager = networkManager; | |||
| InternalBuffer = NetworkBufferPool.GetBuffer(); | |||
| InternalBuffer = new NetworkBuffer(1024, 256); | |||
There was a problem hiding this comment.
How often is this triggered ? Is this a worry for GC ?
There was a problem hiding this comment.
For every NetworkSceneManager instance there will be two (2) SceneEventData instances for the lifetime of that NetworkSceneManager instance. There is always 1 NetworkSceneManager instance per network session (host, server, or client).
So, it will allocate roughly 2048 bytes total when StartHost, StartServer, or StartClient are invoked. One of the two NetworkBuffers could increase in size during client synchronization (i.e. player is just approved and connecting to a session) depending upon how much has to be synchronized.
There was a problem hiding this comment.
I was following till I got here. If we're cleaning ourselves up properly now, can we not go back to just using the PooledNetworkBuffer? If not, can we have a comment saying why SceneEventData needs its own NetworkBuffer?
There was a problem hiding this comment.
Could you hoist what you just commented @NoelStephensUnity into comments in the code?
There was a problem hiding this comment.
I think it's fine to not use the pooled versions in this case (fine either way, honestly)... 2 instances created upon the construction of NetworkManager basically has the same impact as getting 2 from the pool since the pool doesn't pre-allocate any at all.
There was a problem hiding this comment.
Well, it was actual a bit lazy of me to do it that way. This has since been fixed to make SceneEventData use only what it needs and then give the buffer back to the pool when no longer neeed.
| @@ -85,7 +85,7 @@ public class SceneEvent | |||
| /// Main class for managing network scenes when <see cref="NetworkConfig.EnableSceneManagement"/> is enabled. | |||
| /// Uses the <see cref="MessageQueueContainer.MessageType.SceneEvent"/> message to communicate <see cref="SceneEventData"/> between the server and client(s) | |||
| /// </summary> | |||
| public class NetworkSceneManager | |||
| public class NetworkSceneManager:IDisposable | |||
There was a problem hiding this comment.
standards.py is going to be angry with you :)
There was a problem hiding this comment.
Fixed that one standards issue in most recent commit and fixed several other standards issues introduced when metrics were added.
| @@ -754,7 +754,7 @@ public void Dispose() | |||
| internal SceneEventData(NetworkManager networkManager) | |||
| { | |||
| m_NetworkManager = networkManager; | |||
| InternalBuffer = NetworkBufferPool.GetBuffer(); | |||
| InternalBuffer = new NetworkBuffer(1024, 256); | |||
There was a problem hiding this comment.
I think it's fine to not use the pooled versions in this case (fine either way, honestly)... 2 instances created upon the construction of NetworkManager basically has the same impact as getting 2 from the pool since the pool doesn't pre-allocate any at all.
|
Now SceneEventData only gets a PooledNetworkBuffer when needed and places the PooledNetworkBuffer back into the pool when it is no longer needed. So, we just saved 2048 bytes! |
Removed a LF that was not intended. Added comments to SetInternalBuffer and ReleaseInternalBuffer while also making them both private as nothing else needs to be calling these two methods.
…hub.com:Unity-Technologies/com.unity.multiplayer.mlapi into sam/feature/interpolation-for-network-transform * 'sam/feature/interpolation-for-network-transform' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: fix: networkscenemanager not releasing buffers from pool (#1132) test: fixed-length strings in netvars (#1119) fix: snapshot system. last fixes for release (#1129) refactor!: Unified Shutdown (#1108) chore: Fill out unity project for integration test project (#1128) feat: make ServerRpc ownership check an error log instead of warning log (#1126)
…ologies#1132) * fix Don't used pooled NetworkBuffers for SceneEventData, instead just allocate 1 dedicated NetworkBuffer per SceneEventData (which will yield 2 instances per NetworkSceneManager). Make sure we dispose the scene manager so it will dispose the two SceneEventData instances so they will Dispose their NetworkBuffers. * style Adding comments * refactor Switching back to PooledNetworkBuffer. Now it only gets a PooledNetworkBuffer when needed, and releases it back into the pool when it is no longer needed. * style Fixing one standards violation introduced in this PR and several more that were introduced when metrics were added. * style and refactor Removed a LF that was not intended. Added comments to SetInternalBuffer and ReleaseInternalBuffer while also making them both private as nothing else needs to be calling these two methods. * refactor Removing standards check. * style Removing 1 LF from SceneEventData constructor. Slightly improved constructor description. Removed 1 LF between ReleaseInternalBuffer and SetInternalBuffer Removed 1 LF between AddSceneToSynchronize and SceneHandlesToSynchronize property.
This fixes the issue where the two SceneEventData instances in NetworkSceneManager were not putting their PooledNetworkBuffers back into the pool. After further consideration, it seemed better to not hold onto PooledNetworkBuffers for the duration of the NetworkSceneManager's life time.
Included in this fix: