feat: Fast buffer reader and fast buffer writer #1082
Conversation
… some xmldoc comments though)
…(aside from INetworkSerializable, which isn't ready to be tested yet due to lack of BufferSerializer)
- Streamlined the implementations of bit-packing uints and ulongs.
…lacing with byte* allocated directly through UnsafeUtility.Malloc() (same function used under the hood by NativeArray). This is required in order to be able to store pointers to FastBufferReader and FastBufferWriter in order to be able to wrap them with BufferSerializer - NativeArray contains a managed variable in it that disallows taking a pointer to it. And since FBW and FBR are structs, pointers are the only way to wrap them "by reference" in another struct - ref fields aren't allowed even inside ref structs.
…g values in other ref structs in a capture-by-reference style, updated BitReader and BitWriter to use that. Also aggressively inlining properties in FBW and FBR.
ShadauxCat
requested review from
0xFA11,
NoelStephensUnity,
LukeStampfli and
mattwalsh-unity
…. it's a little slower, but apparently some platforms won't support unaligned memory access (e.g., WEBGL, ARM processors) and there's no compile time way to detect ARM processors since the bytecode is not processor-dependent... the cost of the runtime detection would be more expensive than the cost of just doing the memcpy.
…afeUtility.MemCpy for small values, while still supporting unaligned access.
| @@ -0,0 +1,2 @@ | |||
| /BuildInfo.json | |||
There was a problem hiding this comment.
There's already 3+ .gitignores in the repo can we add this to one of those instead of this deeply nested one?
There was a problem hiding this comment.
+1, could add StreamingAssets/ line to testproject/.gitignore file :)
There was a problem hiding this comment.
Sorry about that - my git client defaults to ignoring in the same directory when selecting to ignore files. I'll make sure to edit the .gitignore manually next time instead of using the client.
There was a problem hiding this comment.
wtf... I did delete it... why is it still here?
com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Tests/Editor/Serialization/BitWriterTests.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Serialization/IBufferSerializerImplementation.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Pads the read bit count to byte alignment and commits the read back to the reader | ||
| /// </summary> | ||
| public void Dispose() |
There was a problem hiding this comment.
Did you mean to implement IDisposable?
There was a problem hiding this comment.
ref structs can't implement interfaces, but c# has special behavior to support using() on ref structs that have a public void Dispose()
There was a problem hiding this comment.
Ohh, neat. ref structs are pretty new to me. I'm a little uneasy about some of your changes in general because I haven't dipped into some of the new ref syntax - especially the Ref type you're adding. I hope you're seeking review from some folks that have a deep experience with this since this type of code is very risky
There was a problem hiding this comment.
(To be clear, not saying you shouldn't be doing it, serialization is exactly where I'd expect to be writing this type of code)
com.unity.netcode.gameobjects/Runtime/Serialization/BitWriter.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReaderExtensions.cs
Show resolved
Hide resolved
| public static void ReadValueSafe(this ref FastBufferReader reader, out GameObject value) | ||
| { | ||
| reader.ReadValueSafe(out ulong networkObjectId); | ||
|
|
||
| if (NetworkManager.Singleton.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out NetworkObject networkObject)) | ||
| { | ||
| value = networkObject.gameObject; | ||
| return; | ||
| } |
There was a problem hiding this comment.
we intentionally killed GameObject, NetworkObject & NetworkBehaviour serialization from built-in serializers. we wanted to kill NetworkManager/SpawnManager dependency in a primitive impl like this to keep it isolated and dependency-free (as much as we can).
also we were planning to offer more focused NetworkObject/NetworkBehaviour wrappers which would implement INetworkSerializable interface: struct NetworkObjectReference : INetworkSerializable etc.
There was a problem hiding this comment.
I agree with that approach... I'll look into doing that in the INetworkMessage branch when I refactor INetworkSerializable.
| /// Serializers and Deserializers for FastBufferWriter and FasteBufferReader | ||
| /// SerializersPacked and DeserializersPacked for BytePacker and ByteUnpacker | ||
| /// </summary> | ||
| public static class SerializationTypeTable |
There was a problem hiding this comment.
so we're basically saying you can have completely separate serializers and deserializers for the same type? and even completely separate packed and non-packed serializers and deserializers? (in total 2 * 2 = 4 tables per type)
There was a problem hiding this comment.
Honestly, support for serializing object is only here because I don't know 100% for sure that I can kill it. I'll know when the IMessage stuff is done... And I'm hoping when I get there, I can just delete this file along with ReadObject and WriteObject entirely.
com.unity.netcode.gameobjects/Tests/Editor/Serialization/BitWriterTests.cs
Show resolved
Hide resolved
| /// If true, an extra byte will be written to indicate whether or not the value is null. | ||
| /// Some types will always write this. | ||
| /// </param> | ||
| public static void WriteObjectPacked(ref FastBufferWriter writer, object value, bool isNullable = false) |
There was a problem hiding this comment.
since we're not offering anything automagical under this method here (apart from auto type -> method convenience), would it make more sense to drop this WriteObject/ReadObject combo entirely? I think we could have a convo about this and hopefully I might convince people to do so :)
There was a problem hiding this comment.
See above comment. I'm hoping the answer is yes but don't want to commit to that until I've converted the existing messages without needing it.
- Fixed incorrect text in a warning in FastBufferReader
-Added documentation on PreChecked() functions.
| /// <summary> | ||
| /// Serialize a GameObject | ||
| /// | ||
| /// Throws OverflowException if the end of the buffer has been reached. | ||
| /// Write buffers will grow up to the maximum allowable message size before throwing OverflowException. | ||
| /// </summary> | ||
| /// <param name="value">Value to serialize</param> | ||
| public void SerializeValue(ref GameObject value) | ||
| { | ||
| m_Implementation.SerializeValue(ref value); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Serialize a NetworkObject | ||
| /// | ||
| /// Throws OverflowException if the end of the buffer has been reached. | ||
| /// Write buffers will grow up to the maximum allowable message size before throwing OverflowException. | ||
| /// </summary> | ||
| /// <param name="value">Value to serialize</param> | ||
| public void SerializeValue(ref NetworkObject value) | ||
| { | ||
| m_Implementation.SerializeValue(ref value); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Serialize a NetworkBehaviour | ||
| /// | ||
| /// Throws OverflowException if the end of the buffer has been reached. | ||
| /// Write buffers will grow up to the maximum allowable message size before throwing OverflowException. | ||
| /// </summary> | ||
| /// <param name="value">Value to serialize</param> | ||
| public void SerializeValue(ref NetworkBehaviour value) | ||
| { | ||
| m_Implementation.SerializeValue(ref value); | ||
| } |
There was a problem hiding this comment.
I wouldn't re-introduce these (both here and also reader/writer impls) because we know that we want to have struct NetworkObjectReference etc. for these scenarios. Killing these here and in reader/writer also kills NetworkManager.SpawnManager deps too — so, an easy win-win?
There was a problem hiding this comment.
(I'd also add Read/Write Object to this context too, I remember calling that out earlier but I wanted to call out again to stress a little bit more)
There was a problem hiding this comment.
I'll say the same thing I said before.. I want to finish INetworkMessage implementation before I kill these. If it turns out they're unused/unneeded once everything's done, I'll happily kill them, but I don't want to kill them now and have to resurrect them later. Converting NetworkObject and NetworkBehaviour to be INetworkSerializable should be trivial... GameObject not so much, but we can also just not support game objects and demand that the player send us the NetworkObject.
| ] | ||
| ], | ||
| "excludePlatforms": [], | ||
| "allowUnsafeCode": true, |
There was a problem hiding this comment.
nit: all we need is "allowUnsafeCode": true line manually added here (I don't like Unity auto-gen'ing these things all the time, leaving minimal stuff here makes us more future-proof between different Unity Editor versions)
# Conflicts: # com.unity.netcode.gameobjects/Tests/Editor/com.unity.netcode.editortests.asmdef
… looked at the files they were in?
…ccess other than integrating develop ...
…nsform * develop: feat: Fast buffer reader and fast buffer writer (#1082) # Conflicts: # com.unity.netcode.gameobjects/Tests/Editor/com.unity.netcode.editortests.asmdef
…am/feature/client-network-transform * sam/feature/interpolation-for-network-transform: (22 commits) fixing line issue more formatting fixing formatting issue removing not submitted LiteNetLib from ZooSam feat: Fast buffer reader and fast buffer writer (#1082) restricting public api bumping exec order feat: NetworkBehaviour.IsSpawned (#1190) feat: added tip to the network manager inspector that directs to install tools (MTT-1211) (#1182) refactor!: remove network dictionary & set, use native container in List, add tests (#1149) fix: Fixed remote disconnects not properly cleaning up (#1184) test: base changes to PR-1114 (#1165) test: verify do not destroy networkobjects on networkmanager shutdown (#1183) chore: removal of EnableNetworkVariable in NetworkConfig. It's always True now (#1179) fix: Fix DontDestroyWithOwner not returning ownership (#1181) test: Giving Android some more room as the connection tests are timing sensitive (#1178) fix: unitytransport connectionmode buttons (#1176) test: added min frames to multi-instance helper (#1170) chore: Add mobile tests to nightly trigger (#1161) feat: snapshot spawn pre-requisite (#1166) ... # Conflicts: # com.unity.netcode.gameobjects/Components/NetworkTransform.cs # testproject/Assets/Scenes/SampleScene.unity
) * FastBufferWriter implemented and tested (still need to add and update some xmldoc comments though) * A few additional tests to cover the last missed cases I can think of (aside from INetworkSerializable, which isn't ready to be tested yet due to lack of BufferSerializer) * FastBufferReader + tests * - More tests - Streamlined the implementations of bit-packing uints and ulongs. * - Removed NativeArray from FastBufferReader and FastBufferWriter, replacing with byte* allocated directly through UnsafeUtility.Malloc() (same function used under the hood by NativeArray). This is required in order to be able to store pointers to FastBufferReader and FastBufferWriter in order to be able to wrap them with BufferSerializer - NativeArray contains a managed variable in it that disallows taking a pointer to it. And since FBW and FBR are structs, pointers are the only way to wrap them "by reference" in another struct - ref fields aren't allowed even inside ref structs. * Added utility ref struct "Ref<T>" to more generically support wrapping values in other ref structs in a capture-by-reference style, updated BitReader and BitWriter to use that. Also aggressively inlining properties in FBW and FBR. * BufferSerializer and tests. * Removed unnecessary comment. * XMLDocs + cleanup for PR * Replaced possibly unaligned memory access with UnsafeUtility.MemCpy... it's a little slower, but apparently some platforms won't support unaligned memory access (e.g., WEBGL, ARM processors) and there's no compile time way to detect ARM processors since the bytecode is not processor-dependent... the cost of the runtime detection would be more expensive than the cost of just doing the memcpy. * Resurrected BytewiseUtil.FastCopyBytes as a faster alternative to UnsafeUtility.MemCpy for small values, while still supporting unaligned access. * Reverting an accidental change. * Removed files that got accidentally duplicated from before the rename. * Standards fixes * Removed accidentally added files. * Added BuildInfo.json to the .gitignore so I stop accidentally checking it in. * Addressed most of the review feedback. Still need to do a little more restructuring of some of the other tests. * standards.py --fix * standards.py --fix * Fixed incorrect namespaces. * -Fixed a couple of issues where growing a FastBufferWriter wouldn't work correctly (requesting beyond MaxCapacity and requesting more than double current capacity) -Added support for FastBufferReader to be used in a mode that doesn't copy the input buffer * Fix a test failure and better implementation of large growths * - Removed RefArray - Fixed incorrect text in a warning in FastBufferReader * Removed RefArray meta file that stuck around. * Review feedback: Used nameof() instead of string literal. * -Removed BytewiseUtility.FastCopyBytes -Added documentation on PreChecked() functions. * removed .gitignore. * Fixed compile errors that somehow didn't happen on my machine until I looked at the files they were in? * standards.py --fix ... again ... despite no changes since the last success other than integrating develop ...
Covered by a total of 646 new unit tests.