feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP #875

Rosme · 2021-06-01T19:24:43Z

related to MTT-802

bendoyon · 2021-06-01T19:28:16Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

+                    //IF TOOLS
+                    instructions.Add(processor.Create(OpCodes.Ldsfld, m_NetworkManager_rpc_name_table_FieldRef));
+                    instructions.Add(processor.Create(OpCodes.Ldc_I4, unchecked((int)hash)));
+                    //Removing __nhandler from the name of the static handler


I think this comment should indicate why we're doing this, rather than what we're doing, since the code already tells us the latter.

I think this comment should not exist but instead // NetworkManager.__rpc_name_table.Add(HandlerHash, RpcMethodName); comment should exist on top of this block similar to the previous block.

Ahhh sounds good.

bendoyon · 2021-06-01T19:28:38Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

@@ -235,6 +238,11 @@ private bool ImportReferences(ModuleDefinition moduleDefinition)
                        m_NetworkManager_ntable_FieldRef = moduleDefinition.ImportReference(fieldInfo);
                        m_NetworkManager_ntable_Add_MethodRef = moduleDefinition.ImportReference(fieldInfo.FieldType.GetMethod("Add"));
                        break;
+                    //IF TOOLS


In this case I would suggest only merging this when we have the conditional dependency figure out.

We can update later. This will be in develop until then, and won't be on release path right away

I'm having a hard time getting comfortable with this with the knowledge I have and can get from this code. Evidently this means "we only want this code when we are tools". I'm not sure what "tools" means. Does it mean "when running in the editor"?

If we really need to get this in before we sort out what this means / how we'll block it, I could live with

Suggested change

//IF TOOLS

#ifdef TOOLS_MODE

Where TOOLS_MODE is always false, and then for your development in the meantime you can force it on, vs. having it default on with a TODO to remember to block it later (opt-in now vs. future opt-out). Maybe I'm over paranoid, but remember develop is used now by early-adopter devs.

oh, if that's the case, it should not be on develop branch I'm afraid.
basically, develop branch needs to be stable and ready to cut yet another release anytime.
it might be a little frustrating to keep it up-to-date with the latest changes getting into develop but I think until it's "ready to be shipped" or in other terms, "stable" it should not be merged into the develop branch.
don't panic yet, we can chat about that later internally in our slack channels :)

Why are we trying to strip out this code? It is a static lookup table of a single string for each RPC defined in code. Stripping this out is a micro optimization.

To be more clear, I don't see why this shouldn't go into develop as-is. Do you have concerns about the stability?

I think we should just remove the comment to avoid confusion.

IDK, I think it's safe to have it all the time and fairly simple/low-cost. I agree with you here.
In terms of general stability, I think it looks good and I couldn't see any issues so far (might do the last check when it's ready to be merged).

I'm fine with putting as is. I was under the impression we wanted those only in the case the tools package was there. But if that's not the case, I'll remove those.

bendoyon · 2021-06-01T19:31:29Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

@@ -35,6 +35,9 @@ public class NetworkManager : MonoBehaviour, INetworkUpdateSystem, IProfilableTr
 #if UNITY_2020_2_OR_NEWER
        // RuntimeAccessModifiersILPP will make this `public`
        internal static readonly Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>> __ntable = new Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>>();
+        // IF TOOLS
+        // For debug and profiling purpose only
+        internal static readonly Dictionary<uint, string> __rpc_name_table = new Dictionary<uint, string>();


Is there a way to figure out the name of the RPC as it happens, rather than keeping a map around?

Asking mostly out of curiosity, because I figure if this the solution we ended up with, it's probably the only way.

There's no other way really. We will use this to retrieve the name when invoking RPC.

I just realized it'd be nicer to call __ntable as __rpc_func_table too :)

Want me to do that as part of this PR? I don't think it's a good idea to do it in this PR.

nah, but I will do that soon.

JesseOlmer · 2021-06-01T19:32:02Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

@@ -35,6 +35,9 @@ public class NetworkManager : MonoBehaviour, INetworkUpdateSystem, IProfilableTr
 #if UNITY_2020_2_OR_NEWER
        // RuntimeAccessModifiersILPP will make this `public`
        internal static readonly Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>> __ntable = new Dictionary<uint, Action<NetworkBehaviour, NetworkSerializer, __RpcParams>>();
+        // IF TOOLS
+        // For debug and profiling purpose only


Then this should only happen in debug builds, yes?

@MFatihMAR Wanted to put warnings all over the place. In the next sprint, we will surround those with the tools define. We just need to merge our packages before we have this define in place. Those are more for a reminder to update those place. Or would you rather we wait for that to be done before we merge this PR?

I was talking about #if DEVELOPMENT_BUILD || UNITY_EDITOR so that it's compiled out of release builds, unless you're saying we're going to make a netcode tools define that effectively does the same thing? Would love to hear how that interracts with build settings.

ditto here, also put a similar comment above.
I thought it wouldn't be just #if DEVELOPMENT_BUILD || UNITY_EDITOR wrapped into #if OUR_DEF_TOOLS macro but rather yet another macro to be defined in player/build settings to enable/disable, therefore I suggested keeping it enabled with comment surroundings and wrap with that macro when it lands.
if that's not the case, then yeah, we should wait for other things to merge this into the develop branch (as per my previous comment above).

Like my other comment, I think we should remove the comment rather than add conditionals here. We shouldn't prescribe the use case in the implementation, we should just make the implementation work. I can't see much reason for why "Internally lookup rpc name from the uint identifier at runtime" needs to have "only accessible from debug code" as a requirement.

agreed. since it's internal-only, it would not cause big problems, I wouldn't say the same if it was public to devs though.

Sound good. I'll remove the comments and not put any conditionals.

mattwalsh-unity

Comments on the pre-processor stuff

0xFA11 · 2021-06-02T00:15:12Z

com.unity.multiplayer.mlapi/Editor/CodeGen/RuntimeAccessModifiersILPP.cs

+                //IF TOOLS
+                if (fieldDefinition.Name == nameof(NetworkManager.__rpc_name_table))
+                {
+                    fieldDefinition.IsPublic = true;
+                }


this was a workaround to support 2019.4 which we no longer support, so doesn't need to exist.
(and you just reminded me about a cleanup I'm supposed to do for the next release, no more 2019.4 workarounds, yay!)

Okay, I'll remove that.

becksebenius-unity · 2021-06-02T02:02:24Z

General approach comment: Is there a reason we don't just pass the rpc name as a string parameter to the runtime call method? That way we don't have to keep a static lookup table or do a runtime lookup - the pointer to the interned string literal would exist on the stack for us at no cost and we can add stat reporting code there in an obvious way.

0xFA11 · 2021-06-02T12:35:40Z

@becksebenius-unity

pass the rpc name as a string parameter to the runtime call method

runtime performance :) method call-stack counts (especially in this case it also means that we'll allocate a string too!).

becksebenius-unity · 2021-06-02T16:48:14Z

@becksebenius-unity

pass the rpc name as a string parameter to the runtime call method

runtime performance :) method call-stack counts (especially in this case it also means that we'll allocate a string too!).

I don't think there's a real perf cost here. Parameters are effectively free (especially since strings are just pointers, not passed by value), and we wouldn't need to allocate a string since it would be a const generated by ILPP

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

0xFA11 · 2021-06-04T02:10:21Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

+                    // NetworkManager.__rpc_name_table.Add(HandlerHash, RpcMethodName);
+                    instructions.Add(processor.Create(OpCodes.Ldsfld, m_NetworkManager_rpc_name_table_FieldRef));
+                    instructions.Add(processor.Create(OpCodes.Ldc_I4, unchecked((int)hash)));
+                    instructions.Add(processor.Create(OpCodes.Ldstr, method.Name.Substring(0, method.Name.Length-"__nhandler".Length)));
+                    instructions.Add(processor.Create(OpCodes.Call, m_NetworkManager_rpc_name_table_Add_MethodRef));


having another thought on this, I came back to the idea of kicking this (and the static __rpc_name_table field in NetworkManager class) out from non-editor && non-development builds, in other terms, these stuff should be there in the editor and/or in the development builds only.

if #if UNITY_EDITOR || DEVELOPMENT_BUILD macro wouldn't work here, using Debug.isDebugBuild and Application.isEditor APIs at ILPP runtime would help.

what do you think @Rosme ?

We're doing to have some tooling for non-development builds (realtime stats monitor, etc)

We can probably include it only by defining only if the conditional dependency of the tooling library is present. If a dev wanted to 100% strip out tooling, they can just remove the lib, but we expect some of them to want this functionality even in non-development builds.

would non-dev builds need RPC string names too?

@becksebenius-unity can probably answer more eloquently and with more insight than I can :)

I'm not seeing the performance concern here. Static constructors are called once-ever for a given class. Dictionary.Add isn't an expensive operation - it's O(1). The memory cost is a single string name for each RPC defined in code - and in fact because this is generated from ILPP, they should be interned by the compiler and not allocated at runtime (even in the constructor). The interned string may even already exist to enable method reflection. I'm not convinced that the code complexity of conditionally running this code is worth it to remove the extremely small runtime impact caused by building a small dictionary.

@becksebenius-unity interning can't happen automatically here because we modify the dll post compile which means string literals injects aren't going to get interned since the compiler is out of it at this point and they won't get added to the intern pool (this is one of the reasons we should get to source code generators at some point) so that means each string is a ldstr opcode that is not interned which means its going to load a copy of the string when it does the add and thus we garbage is generated and a GC will happen for the temp string and further references to the string will result in copies since the interning is not happening.

Now we could do a manual intern with string.intern which would ensure that, but I have just further issue with your statement, right now we are not using these names and they are not free, they grow unbounded in that the amount of them is N where N is the number of unique RPCs in an application. They are eating up memory that is not being used and I have no way of freeing or clearing that memory and I can assure you any dev is going to say ask why do I have this giant dictionary of strings that does nothing and I don't need 90% of the time.

To add to that C# static constructors are lazy which means they do not get called until the type is used or instantiated, which means this hit does potentially come at any time during the lifetime of the game/application. So when you add this together its not free by any means especially with the lack of interning on the string.

So I would at least like to ensure that this code doesn't run when the user doesn't want it to run and especially given it has no value at the moment and I can't see devs wanting this overhead especially on servers and mobile devices in production I am just not seeing what we should not either generate this code unless you are in the editor or a dev build or check at runtime.

Thanks for correcting my assumption about interning, it makes sense that this happens in IL and therefore won't automatically intern during weaving. Although unless I'm misunderstanding, I think your statement that further references to these strings will result in copies, these will strings would only be allocated once and just the pointer will be passed around.

It sounds like you guys feel strongly about the performance cost here. This smells a bit of preoptimization (I still think the cost is being overstated) but I'm not going to push on this if there is consensus. This will mean that we will be inconsistently applying debug-only rules to the collection of metrics and that adds complexity, although admittedly it is extremely unlikely that we would ever have a use for this data in release builds. We'll need to figure out a good way to document which metrics are release-accessible and which aren't so that this doesn't cause confusion down the road

So I don't disagree with you that this is a micro-optimization; but that is not even my primary concern my main issue is actually consistent design principles that we apply to the code base. One of those principles should be that we are ensuring that code that is executing is actually adding value under all circumstances by which the code should be executing.

This code doesn't pass that test, it executes even when the user may not be actually benefiting from or using it, which to me says the code is optional and thus should not be imposed on the user but in fact truly be optional.

If we just allow it to be there for the sake of being there regardless if its being used or not in execution that to me sets a bad precedent for future code and to me it says to our users that we don't care about your resources and we are just going to waste cycles/memory and give you no options to disable the functionality.

Sorry for the late response. I have read everything. In my opinion, as of now, there is no reason to have those strings in non debug build. I would rather have them removed from the release build, and if needed, find a solution to put them back in an efficient way if we need them in release build. While it may be a micro-optimization, as @wackoisgod explained, it's a matter of consistences and there is still a small cost. Might as well limit that for now.

mattwalsh-unity

My concerns have been addressed; for my part I'm fine so long as @MFatihMAR / @becksebenius-unity / @Rosme come to a consensus on perf

0xFA11 · 2021-06-05T00:49:57Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

+                    // NetworkManager.__rpc_name_table.Add(HandlerHash, RpcMethodName);
+                    instructions.Add(processor.Create(OpCodes.Ldsfld, m_NetworkManager_rpc_name_table_FieldRef));
+                    instructions.Add(processor.Create(OpCodes.Ldc_I4, unchecked((int)hash)));
+                    instructions.Add(processor.Create(OpCodes.Ldstr, method.Name.Substring(0, method.Name.Length-"__nhandler".Length)));


instead of doing this grab xxx__handler and cut __handler part trick here, we could grab the name from methodDefinition + methodDefHash above and inject outside this foreach block with another foreach iterating over rpcMethodHash-rpcMethodName combo.
I can reach out later next week to refactor this together.

I tried something like that, but I couldn't get it to work for some reason. I'm all open for better solution.

Updated the naming around to better identify the two lists

Rosme · 2021-06-07T16:43:34Z

I'm adding debug surrounding on the next commit. Forgot it on this.

0xFA11 · 2021-06-07T21:55:07Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

@@ -533,7 +536,14 @@ private bool ImportReferences(ModuleDefinition moduleDefinition)
        private void ProcessNetworkBehaviour(TypeDefinition typeDefinition)
        {
            var staticHandlers = new List<(uint RpcHash, MethodDefinition RpcMethod)>();
-            var rpcsNameMapping = new List<(uint RpcHash, string RpcName)>();
+
+            bool isDebugOrInEditor = (m_CompiledAssembly.Defines.Contains("UNITY_EDITOR") || m_CompiledAssembly.Defines.Contains("DEVELOPMENT_BUILD"));


0xFA11 · 2021-06-07T21:57:09Z

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs

@@ -533,7 +536,14 @@ private bool ImportReferences(ModuleDefinition moduleDefinition)
        private void ProcessNetworkBehaviour(TypeDefinition typeDefinition)


minor: following the similar pattern as RuntimeAccessModifiersILPP, we might just pass string[] assemblyDefines around instead of pinning m_CompiledAssembly to a local variable.

0xFA11 · 2021-06-07T22:32:19Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

        // RuntimeAccessModifiersILPP will make this `public`
        internal static readonly Dictionary<uint, string> __rpc_name_table = new Dictionary<uint, string>();
+#else
+        // RuntimeAccessModifiersILPP will make this `public`


in fact, this won't be processed by RuntimeAccessModifiersILPP according to the changes in RuntimeAccessModifiersILPP.cs.
I think we should either find a way to avoid this (hardcoding string name of this field might not be the best) or remove this comment line to reflect what's implemented (sounds like we'd maintain some strings manually either way :P, it's just comment vs field name).

I think we should keep this as is, but update the comment to better reflect what's happening. I will do that.

Updated the comment in network manager to better reflect what is going on.

Rosme · 2021-06-08T14:52:47Z

@MFatihMAR Want to take a final look maybe? :D

* develop: (21 commits) feat: NetworkObject Parenting (#855) refactor: move RpcMethodId serialization from ILPP to Core (#910) fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905) feat!: OnNetworkSpawn / OnNetworkDespawn (#865) feat: Add missing XMLdoc comment (#897) refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895) fix: do not access/render runtime info if not playing in the editor (#898) feat: Add name property for network variables (#891) chore: delete PhilTestResults.xml (#894) feat: MultiInstanceHelpers to use fixed FrameRate by default (#893) test: General MultiInstanceHelper improvements (#885) refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886) docs: adding more info to help debug on network transform error message (#892) feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875) fix: remove OnClientConnectedCallback registration from StatsDisplay (#882) feat: Add profiling decorator pattern (#878) refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880) fix: update package version to 0.2.0 because of unity minversion change (#881) docs: Fix typo in changelog version title docs: Hotfix Changelog for 0.1.1 and manual update (#873) ... # Conflicts: # com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs # com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs # com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs.meta

* develop: (67 commits) feat: NetworkObject Parenting (#855) refactor: move RpcMethodId serialization from ILPP to Core (#910) fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905) feat!: OnNetworkSpawn / OnNetworkDespawn (#865) feat: Add missing XMLdoc comment (#897) refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895) fix: do not access/render runtime info if not playing in the editor (#898) feat: Add name property for network variables (#891) chore: delete PhilTestResults.xml (#894) feat: MultiInstanceHelpers to use fixed FrameRate by default (#893) test: General MultiInstanceHelper improvements (#885) refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886) docs: adding more info to help debug on network transform error message (#892) feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875) fix: remove OnClientConnectedCallback registration from StatsDisplay (#882) feat: Add profiling decorator pattern (#878) refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880) fix: update package version to 0.2.0 because of unity minversion change (#881) docs: Fix typo in changelog version title docs: Hotfix Changelog for 0.1.1 and manual update (#873) ... # Conflicts: # com.unity.multiplayer.mlapi/Tests/Runtime/com.unity.multiplayer.mlapi.runtimetests.asmdef # testproject/ProjectSettings/EditorBuildSettings.asset

Adding RPC Name Information through ILPP and Code Gen (MTT-802)

c8a5a22

Rosme requested review from 0xFA11 and a team June 1, 2021 19:24

bendoyon reviewed Jun 1, 2021

View reviewed changes

JesseOlmer reviewed Jun 1, 2021

View reviewed changes

mattwalsh-unity suggested changes Jun 2, 2021

View reviewed changes

0xFA11 reviewed Jun 2, 2021

View reviewed changes

0xFA11 changed the title ~~Adding RPC Name Information through ILPP and Code Gen (MTT-802)~~ feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (MTT-802) Jun 2, 2021

0xFA11 changed the title ~~feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (MTT-802)~~ feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP Jun 2, 2021

Updated based on comments and feedback

f0bcc86

0xFA11 reviewed Jun 3, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs Outdated Show resolved Hide resolved

Updating comment to reflect better

ca5029c

0xFA11 reviewed Jun 4, 2021

View reviewed changes

mattwalsh-unity approved these changes Jun 4, 2021

View reviewed changes

0xFA11 reviewed Jun 5, 2021

View reviewed changes

Setup a list to avoid manipulating the string.

6c65dc6

Updated the naming around to better identify the two lists

Using compiled assembly defines list instead of macros

90c3311

wackoisgod approved these changes Jun 7, 2021

View reviewed changes

0xFA11 reviewed Jun 7, 2021

View reviewed changes

0xFA11 and others added 2 commits June 7, 2021 23:33

Merge branch 'develop' into feature/rpc-debugging-info

8826b8b

Passing defines as argument instead of storing them.

4e48166

Updated the comment in network manager to better reflect what is going on.

minor refactor

ec28055

0xFA11 approved these changes Jun 8, 2021

View reviewed changes

0xFA11 enabled auto-merge (squash) June 8, 2021 16:46

0xFA11 merged commit 3e96cf9 into develop Jun 8, 2021

0xFA11 deleted the feature/rpc-debugging-info branch June 8, 2021 17:41

		@@ -533,7 +536,14 @@ private bool ImportReferences(ModuleDefinition moduleDefinition)
		private void ProcessNetworkBehaviour(TypeDefinition typeDefinition)

feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP #875

feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP #875

Uh oh!

Conversation

Rosme commented Jun 1, 2021 • edited by 0xFA11 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 Jun 2, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

JesseOlmer Jun 1, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity 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!

becksebenius-unity commented Jun 2, 2021

Uh oh!

0xFA11 commented Jun 2, 2021

Uh oh!

becksebenius-unity commented Jun 2, 2021

Uh oh!

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

Rosme commented Jun 1, 2021 •

edited by 0xFA11

Loading

0xFA11 Jun 2, 2021 •

edited

Loading

JesseOlmer Jun 1, 2021 •

edited

Loading

bendoyon Jun 4, 2021 •

edited

Loading