feat: QoL: Byte Packing of integers [MTT-4924] #2276

ShadauxCat · 2022-10-25T17:51:46Z

Adds byte packing for most integer fields in the SDK (a few, such as hashes, are left un-packed because they're likely to be large integers and not benefit - and may possibly actually be harmed - by variable-length encoding). Integers in RPCs and NetworkVariables are also packed properly. No functionality has been added to allow a value to be sent unpacked in RPCs or NetworkVariables in cases where packing may be detrimental, but this can be achieved as follows:

public struct UnpackedInt32 : INetworkSerializeByMemcpy
{
    int Value;
}

public NetworkVariable<UnpackedInt32> MyVariable;

In addition to byte packing most integer fields, this PR updates WriteValueBitPacked() to be able to handle the full range of its type (now using an extra byte - and thus ending up making the representation larger instead of smaller - for values at the extreme ends of the size), at the cost of using one extra bit to encode the size of the type. WriteValueBitPacked shrinks values as follows:

16-bit integers:
- 0-6 bits used: 1 byte
- 7-14 bits used: 2 bytes
- 15-16 bits used: 3 bytes
32-bit integers:
- 0-5 bits used: 1 byte
- 6-13 bits used: 2 bytes
- 14-21 bits used: 3 bytes
- 22-29 bits used: 4 bytes
- 30-32 bits used: 5 bytes
64-bit integers:
- 0-4 bits used: 1 byte
- 5-12 bits used: 2 bytes
- 13-20 bits used: 3 bytes
- 21-28 bits used: 4 bytes
- 29-36 bits used: 5 bytes
- 37-44 bits used: 6 bytes
- 45-52 bits used: 7 bytes
- 53-60 bits used: 8 bytes
- 61-64 bits used: 9 bytes

Because this provides better compression than WriteValuePacked in all cases and no longer has any size limits, the old implementation for WriteValuePacked has been removed and all calls to WriteValuePacked now simply forward to WriteValueBitPacked.

This scheme was chosen over the standard Variable-Length Quantity representation for two reasons:

It's less computationally expensive than VLQ (as VLQ has to mask out a bit from every single byte and combine multiple 7-bit values into a single 16-, 32-, or 64-bit value), and
While VLQ can achieve better compression with small values, it scales poorly by compression - a full-size 64-bit integer ends up using 10 bytes with VLQ, rather than 9 bytes with this algorithm (or its native 8 bytes)

Here are some graphs that show the comparisons (the lower the value the better the compression; the further to the right the value changed, the better the compression scales). WriteValuePacked here represents the old method, which has been removed in this PR.

Changelog

Changed: Optimized bandwidth usage by encoding most integer fields using variable-length encoding.

Testing and Documentation

No tests have been added (as no functionality was added or changed, only optimized).
No documentation changes or additions were necessary.

…on it broke some tests...), revert some debug logs

…LogMessage in tools test, because packing makes the actual expected message size non-trivial to calculate.

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs

jeffreyrainy · 2022-10-28T15:39:19Z

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs

@@ -356,11 +342,17 @@ public static unsafe void ReadValueBitPacked(FastBufferReader reader, out ushort
                    *ptr = *data;
                    *(ptr + 1) = *(data + 1);
                    break;
+                case 3:
+                    // First byte contains no data, it's just a marker. The data is in the remaining two bytes.
+                    *ptr = *(data + 1);


same as above *(ptr++) = *(data++); twice, with extra ++data at beginning ? maybe it's just style.

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs

jeffreyrainy · 2022-10-28T15:42:26Z

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs

@@ -500,58 +500,23 @@ public static unsafe void ReadValueBitPacked(FastBufferReader reader, out ulong
                    *(ptr + 6) = *(data + 6);
                    *(ptr + 7) = *(data + 7);
                    break;
+                case 9:
+                    // First byte contains no data, it's just a marker. The data is in the remaining two bytes.
+                    *ptr = *(data + 1);


Looks like a manually unrolled loop. :-) Is there any higher-level construct that will instruct to compiler to copy those 8 bytes that is easier to read and/or doesn't go so squarely against the DRY principle?

IIRC when I was writing this, I did performance comparisons between:

UnsafeUtility.MemCpy

A for() loop

And a manually unrolled loop

And the manually unrolled loop was measurably faster. Normally I'd say "don't worry about that, clarity's more important", but given that this is something we do potentially dozens to hundreds to even thousands of times per frame depending how many NetworkVariables are out there and how much they're changing, this is one area of code where I'm personally inclined to err on the side of optimization over clarity.

manually unrolled loop was measurably faster

even in standalone release builds too?

Yeah, I was testing in both mono and IL2CPP in standalone builds.

jeffreyrainy

approved with some minor stylistic nits

NoelStephensUnity · 2022-10-31T17:05:07Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

+                    // Note: This value can't be packed because we don't know how large it will be in advance
+                    // we reserve space for it, then write the data, then come back and fill in the space
+                    // to pack here, we'd have to write data to a temporary buffer and copy it in - which
+                    // isn't worth possibly saving one byte IFF the data is less than 63 bytes long...


Nit-pick or curious what it means:
IFF? abbreviation or misspelled?

Haha, sorry, using formal logical language... IFF is shorthand for "If And Only If"

https://en.wikipedia.org/wiki/If_and_only_if

I'll change it.

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

com.unity.netcode.gameobjects/Runtime/Serialization/BytePacker.cs

NoelStephensUnity · 2022-10-31T19:37:29Z

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -692,6 +699,18 @@ protected void DestroySceneNetworkObjects()
            }
        }

+        /// <summary>


😻
Awesome! Nice addition!

NoelStephensUnity · 2022-10-31T19:39:17Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs

@@ -842,7 +841,7 @@ private bool RotationMatchesValue(Vector3 rotationEulerToMatch)
            }
            if (!nonauthorityIsEqual)
            {
-                VerboseDebug($"NonAuthority position {nonAuthorityRotationEuler} != rotation to match: {rotationEulerToMatch}!");
+                VerboseDebug($"NonAuthority rotation {nonAuthorityRotationEuler} != rotation to match: {rotationEulerToMatch}!");


Nice copy paste debug output catch!

NoelStephensUnity

Other than the following:

The 8 to 5 bytes for the SceneObject structure
This could possibly need to be updated as a minor version feat! due to the deprecated public const values.

The rest looks good to me, will review again once you come to a conclusion about the above two items...I will do a second pass over it all once more.

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/ServerLogsMetricTests.cs

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/OwnershipChangeMetricsTests.cs

ShadauxCat · 2022-11-04T19:50:20Z

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/ServerLogsMetricTests.cs

-        private const int k_MessageHeaderSize = 2;
-        private static readonly int k_ServerLogSentMessageOverhead = 2 + k_MessageHeaderSize;
-        private static readonly int k_ServerLogReceivedMessageOverhead = 2;
+        // Header is dynamically sized due to packing, will be 3 bytes for all test messages.


Note: This increased from 2 bytes to 3 bytes here because BytePacker.WriteValueBitPacked was changed to reserve 3 bits instead of 2 for a uint. It used to be that message length <= 63 would take 1 byte, now message length <= 31 does. The message length for a guid is consistently in between those sizes, so the message header for this test is now 3 bytes.

Rosme

Thanks for this change. Looks good!

# Conflicts: # com.unity.netcode.gameobjects/CHANGELOG.md # com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

Adds byte packing for most integer fields in the SDK (a few, such as hashes, are left un-packed because they're likely to be large integers and not benefit - and may possibly actually be harmed - by variable-length encoding). Integers in `RPC`s and `NetworkVariable`s are also packed properly. No functionality has been added to allow a value to be sent unpacked in `RPC`s or `NetworkVariable`s in cases where packing may be detrimental, but this can be achieved as follows: ```csharp public struct UnpackedInt32 : INetworkSerializeByMemcpy { int Value; } public NetworkVariable<UnpackedInt32> MyVariable; ```` In addition to byte packing most integer fields, this commit updates `WriteValueBitPacked()` to be able to handle the full range of its type (now using an extra byte - and thus ending up making the representation larger instead of smaller - for values at the extreme ends of the size), at the cost of using one extra bit to encode the size of the type.

ShadauxCat added 8 commits October 24, 2022 14:13

byte packing WIP

3f96d26

Merge branch 'develop' into feat/bytepacking

917840b

Revert change to TimeSyncMessage.Tick from int to long (for some reas…

61aa05e

…on it broke some tests...), revert some debug logs

Fixes

2f27edd

Revert unrelated changes.

5d97da0

Style

d55b97b

Cleanup

f8d4ad5

Fixed error in comments

e4716d0

ShadauxCat requested a review from a team as a code owner October 25, 2022 17:51

ShadauxCat added 4 commits October 25, 2022 12:58

changelog entry

480ec13

standards again, apparently?

df90492

Hoisted some common code into functions

3217a21

Removed asserts on message size for ChangeOwnershipMessage and Server…

8c6c3e8

…LogMessage in tools test, because packing makes the actual expected message size non-trivial to calculate.

ShadauxCat requested a review from a team as a code owner October 25, 2022 20:51

ShadauxCat added 2 commits October 25, 2022 17:10

Fix isolation tests...

9d7ed31

standards

5481c45

0xFA11 approved these changes Oct 26, 2022

View reviewed changes

Merge branch 'develop' into feat/bytepacking

ddeb80a

jeffreyrainy reviewed Oct 28, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs Outdated Show resolved Hide resolved

jeffreyrainy reviewed Oct 28, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Serialization/ByteUnpacker.cs Show resolved Hide resolved

jeffreyrainy reviewed Oct 28, 2022

View reviewed changes

jeffreyrainy approved these changes Oct 28, 2022

View reviewed changes

NoelStephensUnity reviewed Oct 31, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs Outdated Show resolved Hide resolved

NoelStephensUnity reviewed Oct 31, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Serialization/BytePacker.cs Show resolved Hide resolved

NoelStephensUnity reviewed Oct 31, 2022

View reviewed changes

NoelStephensUnity approved these changes Oct 31, 2022

View reviewed changes

ShadauxCat added 2 commits November 1, 2022 17:19

review feedback

f994c31

Hash actually shouldn't be packed.

eaa914c

Rosme suggested changes Nov 2, 2022

View reviewed changes

Reverting changes to test assertions.

63d4e61

ShadauxCat commented Nov 4, 2022

View reviewed changes

Rosme approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'develop' into feat/bytepacking

bc2c1a9

# Conflicts: # com.unity.netcode.gameobjects/CHANGELOG.md # com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

ShadauxCat enabled auto-merge (squash) November 7, 2022 17:05

Merge develop into feat/bytepacking

b4063d7

ShadauxCat merged commit bca25b9 into develop Nov 8, 2022

ShadauxCat deleted the feat/bytepacking branch November 8, 2022 16:57

feat: QoL: Byte Packing of integers [MTT-4924] #2276

feat: QoL: Byte Packing of integers [MTT-4924] #2276

Uh oh!

Conversation

ShadauxCat commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Testing and Documentation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeffreyrainy Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreyrainy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShadauxCat commented Oct 25, 2022 •

edited

Loading

jeffreyrainy Oct 28, 2022 •

edited

Loading