Skip to content

fix: if-guard NetworkManager.__rpc_name_table access #1056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
0xFA11 merged 1 commit into from
Aug 16, 2021
Merged

fix: if-guard NetworkManager.__rpc_name_table access #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,12 @@ internal void __endSendServerRpc(NetworkSerializer serializer, uint rpcMethodId,
? NetworkManager.MessageQueueContainer.EndAddQueueItemToFrame(serializer.Writer, MessageQueueHistoryFrame.QueueFrameType.Inbound, serverRpcParams.Send.UpdateStage)
: NetworkManager.MessageQueueContainer.EndAddQueueItemToFrame(serializer.Writer, MessageQueueHistoryFrame.QueueFrameType.Outbound, NetworkUpdateStage.PostLateUpdate);

#if DEVELOPMENT_BUILD || UNITY_EDITOR
Copy link
Copy Markdown
Contributor

@mattwalsh-unity mattwalsh-unity Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely fixes it for me.

I'm assuming it's the NetworkManager.NetworkMetrics is null?

What I don't know is if the tools team indeed intended this to be dev / editor only. I dunno, maybe they would want to be able to track RPCs in a production app? @becksebenius-unity ?

Copy link
Copy Markdown
Contributor

@becksebenius-unity becksebenius-unity Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We compromised on this one because there was a fairly long-winded discussion about whether the additional memory footprint of rpc names should exist in release builds - and since the Profiler (the only tool being released with this version) is only enabled in Development builds, we decided to close the conversation and go this route. It does seem fairly unlikely that we would need the names of RPCs reported in any tool other than the profiler, but that remains to be seen.

Copy link
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might bring them back once we justify their use-cases and even then, we might follow a different approach to reduce memory footprint (thinking aggressive inlining here).

if (NetworkManager.__rpc_name_table.TryGetValue(rpcMethodId, out var rpcMethodName))
{
NetworkManager.NetworkMetrics.TrackRpcSent(NetworkManager.ServerClientId, NetworkObjectId, rpcMethodName, rpcMessageSize);
}
#endif
}

#pragma warning disable IDE1006 // disable naming rule violation check
Expand Down Expand Up @@ -207,10 +209,12 @@ internal void __endSendClientRpc(NetworkSerializer serializer, uint rpcMethodId,

var messageSize = NetworkManager.MessageQueueContainer.EndAddQueueItemToFrame(serializer.Writer, MessageQueueHistoryFrame.QueueFrameType.Outbound, NetworkUpdateStage.PostLateUpdate);

#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (NetworkManager.__rpc_name_table.TryGetValue(rpcMethodId, out var rpcMethodName))
{
NetworkManager.NetworkMetrics.TrackRpcSent(NetworkManager.ConnectedClients.Select(x => x.Key).ToArray(), NetworkObjectId, rpcMethodName, messageSize);
}
#endif
}

/// <summary>
Expand Down
12 changes: 5 additions & 7 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,11 @@ public class NetworkManager : MonoBehaviour, INetworkUpdateSystem, IProfilableTr
// RuntimeAccessModifiersILPP will make this `public`
internal static readonly Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>> __rpc_func_table = new Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>>();

#if UNITY_EDITOR || DEVELOPMENT_BUILD

#if DEVELOPMENT_BUILD || UNITY_EDITOR
// RuntimeAccessModifiersILPP will make this `public`
internal static readonly Dictionary<uint, string> __rpc_name_table = new Dictionary<uint, string>();
#else // !(UNITY_EDITOR || DEVELOPMENT_BUILD)
// RuntimeAccessModifiersILPP will make this `public`
internal static readonly Dictionary<uint, string> __rpc_name_table = null; // not needed on release builds
Copy link
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we kept this field for the old ILPP implementation (pre-2020.3 ILPP, targeting 2019.4) because we needed it for nameof(NetworkManager.__rpc_name_table) code.
however, ILPP code is refactored (cleaned) on 2020.3+ and now we don't actually require this field to be there in the middle of ILPP.
so, let's get rid of it and wrap lines using __rpc_name_table with #if DEVELOPMENT_BUILD || UNITY_EDITOR everywhere!

#endif // UNITY_EDITOR || DEVELOPMENT_BUILD
#endif

#pragma warning restore IDE1006 // restore naming rule violation check

#if DEVELOPMENT_BUILD || UNITY_EDITOR
Expand All @@ -35,7 +32,6 @@ public class NetworkManager : MonoBehaviour, INetworkUpdateSystem, IProfilableTr
private static ProfilerMarker s_TransportConnect = new ProfilerMarker($"{nameof(NetworkManager)}.TransportConnect");
private static ProfilerMarker s_HandleIncomingData = new ProfilerMarker($"{nameof(NetworkManager)}.{nameof(HandleIncomingData)}");
private static ProfilerMarker s_TransportDisconnect = new ProfilerMarker($"{nameof(NetworkManager)}.TransportDisconnect");

private static ProfilerMarker s_InvokeRpc = new ProfilerMarker($"{nameof(NetworkManager)}.{nameof(InvokeRpc)}");
#endif

Expand Down Expand Up @@ -1263,10 +1259,12 @@ internal void InvokeRpc(MessageFrameItem item, NetworkUpdateStage networkUpdateS

__rpc_func_table[networkMethodId](networkBehaviour, new NetworkSerializer(item.NetworkReader), rpcParams);

#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (__rpc_name_table.TryGetValue(networkMethodId, out var rpcMethodName))
{
NetworkMetrics.TrackRpcReceived(item.NetworkId, networkObjectId, rpcMethodName, item.StreamSize);
}
#endif
}
}
}
Expand Down