Skip to content
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

perf: SyncVar hook invocations no longer instantiate a new action delegate on every call #3615

Merged
merged 2 commits into from
Dec 6, 2023
Merged

perf: SyncVar hook invocations no longer instantiate a new action delegate on every call #3615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc6f8ee
Merge pull request #1 from MirrorNetworking/master
cshunsinger Sep 21, 2023
1d9a789
SyncVar hook invocations no longer instantiate a new action delegate …
ClaytonHunsinger Sep 21, 2023
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
28 changes: 21 additions & 7 deletions Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ class NetworkBehaviourProcessor
List<FieldDefinition> syncObjects = new List<FieldDefinition>();
// <SyncVarField,NetIdField>
Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds = new Dictionary<FieldDefinition, FieldDefinition>();
// <SyncVarHookDelegateField, (FieldDefinition, MethodDefinition)> - Every syncvar with a hook has a new field created to store the Action<T,T> delegate so we don't allocate on every hook invocation
// This dictionary maps each syncvar field to the field that will store the hook method delegate instance, and the method from which the delegate instance is constructed from
Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)> syncVarHookDelegates = new Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)>();
readonly List<CmdResult> commands = new List<CmdResult>();
readonly List<ClientRpcResult> clientRpcs = new List<ClientRpcResult>();
readonly List<MethodDefinition> targetRpcs = new List<MethodDefinition>();
Expand Down Expand Up @@ -71,7 +74,7 @@ public bool Process(ref bool WeavingFailed)
MarkAsProcessed(netBehaviourSubclass);

// deconstruct tuple and set fields
(syncVars, syncVarNetIds) = syncVarAttributeProcessor.ProcessSyncVars(netBehaviourSubclass, ref WeavingFailed);
(syncVars, syncVarNetIds, syncVarHookDelegates) = syncVarAttributeProcessor.ProcessSyncVars(netBehaviourSubclass, ref WeavingFailed);

syncObjects = SyncObjectProcessor.FindSyncObjectsFields(writers, readers, Log, netBehaviourSubclass, ref WeavingFailed);

Expand Down Expand Up @@ -321,7 +324,7 @@ void InjectIntoStaticConstructor(ref bool WeavingFailed)
// we need to inject several initializations into NetworkBehaviour ctor
void InjectIntoInstanceConstructor(ref bool WeavingFailed)
{
if (syncObjects.Count == 0)
if ((syncObjects.Count == 0) && (syncVarHookDelegates.Count == 0))
return;

// find instance constructor
Expand Down Expand Up @@ -349,6 +352,14 @@ void InjectIntoInstanceConstructor(ref bool WeavingFailed)
SyncObjectInitializer.GenerateSyncObjectInitializer(ctorWorker, weaverTypes, fd);
}

// initialize all delegate fields in ctor
foreach(KeyValuePair<FieldDefinition, (FieldDefinition, MethodDefinition)> entry in syncVarHookDelegates)
{
FieldDefinition syncVarField = entry.Key;
(FieldDefinition hookDelegate, MethodDefinition hookMethod) = entry.Value;
syncVarAttributeProcessor.GenerateSyncVarHookDelegateInitializer(ctorWorker, syncVarField, hookDelegate, hookMethod);
}

// add final 'Ret' instruction to ctor
ctorWorker.Append(ctorWorker.Create(OpCodes.Ret));
}
Expand Down Expand Up @@ -582,15 +593,18 @@ void DeserializeField(FieldDefinition syncVar, ILProcessor worker, ref bool Weav
worker.Emit(OpCodes.Ldflda, syncVar);
}

// hook? then push 'new Action<T,T>(Hook)' onto stack
MethodDefinition hookMethod = syncVarAttributeProcessor.GetHookMethod(netBehaviourSubclass, syncVar, ref WeavingFailed);
if (hookMethod != null)
// If a hook exists, then we need to load the hook delegate on the stack
// The hook delegate is created once in the constructor and stored in an instance field
// We load the delegate from this instance field to avoid instantiating a new delegate instance every time (drastically reduces allocations)
if(syncVarHookDelegates.TryGetValue(syncVar, out (FieldDefinition hookDelegateField, MethodDefinition) value))
{
syncVarAttributeProcessor.GenerateNewActionFromHookMethod(syncVar, worker, hookMethod);
// A hook exists. Push this.hookDelegateField onto the stack
worker.Emit(OpCodes.Ldarg_0);
worker.Emit(OpCodes.Ldfld, value.hookDelegateField);
}
// otherwise push 'null' as hook
else
{
// No hook exists. Push 'null' as hook
worker.Emit(OpCodes.Ldnull);
}

Expand Down
58 changes: 45 additions & 13 deletions Assets/Mirror/Editor/Weaver/Processors/SyncVarAttributeProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,26 @@ public MethodDefinition GetHookMethod(TypeDefinition td, FieldDefinition syncVar
return FindHookMethod(td, syncVar, hookFunctionName, ref WeavingFailed);
}

// Create a field definition for a field that will store the Action<T, T> delegate instance for the syncvar hook method (only instantiate delegate once)
public FieldDefinition CreateNewActionFieldDefinitionFromHookMethod(FieldDefinition syncVarField)
{
TypeReference actionRef = assembly.MainModule.ImportReference(typeof(Action<,>));
GenericInstanceType syncVarHookActionDelegateType = actionRef.MakeGenericInstanceType(syncVarField.FieldType, syncVarField.FieldType);
string syncVarHookDelegateFieldName = $"_Mirror_SyncVarHookDelegate_{syncVarField.Name}";
return new FieldDefinition(syncVarHookDelegateFieldName, FieldAttributes.Public, syncVarHookActionDelegateType);
}

// push hook from GetHookMethod() onto the stack as a new Action<T,T>.
// allows for reuse without handling static/virtual cases every time.
// perf warning: it is recommended to use this method only when generating IL to create a new Action<T, T>() in order to store it into a field
// avoid using this to emit IL to instantiate a new action instance every single time one is needed for the same method
public void GenerateNewActionFromHookMethod(FieldDefinition syncVar, ILProcessor worker, MethodDefinition hookMethod)
{
// IL_000a: ldarg.0
// IL_000b: ldftn instance void Mirror.Examples.Tanks.Tank::ExampleHook(int32, int32)
// IL_0011: newobj instance void class [netstandard]System.Action`2<int32, int32>::.ctor(object, native int)

// we support static hook sand instance hooks.
// we support static hooks and instance hooks.
if (hookMethod.IsStatic)
{
// for static hooks, we need to push 'null' first.
Expand Down Expand Up @@ -95,15 +106,23 @@ public void GenerateNewActionFromHookMethod(FieldDefinition syncVar, ILProcessor

// call 'new Action<T,T>()' constructor to convert the function to an action
// we need to make an instance of the generic Action<T,T>.
//
// TODO this allocates a new 'Action' for every SyncVar hook call.
// we should allocate it once and store it somewhere in the future.
// hooks are only called on the client though, so it's not too bad for now.
TypeReference actionRef = assembly.MainModule.ImportReference(typeof(Action<,>));
GenericInstanceType genericInstance = actionRef.MakeGenericInstanceType(syncVar.FieldType, syncVar.FieldType);
worker.Emit(OpCodes.Newobj, weaverTypes.ActionT_T.MakeHostInstanceGeneric(assembly.MainModule, genericInstance));
}

// generates CIL to set an Action<T,T> instance field to a new Action<T,T>(hookMethod)
// this.hookDelegate = new Action<T, T>(HookMethod);
public void GenerateSyncVarHookDelegateInitializer(ILProcessor worker, FieldDefinition syncVar, FieldDefinition hookDelegate, MethodDefinition hookMethod)
{
// push this
worker.Emit(OpCodes.Ldarg_0);
// push new Action<T, T>(hookMethod)
GenerateNewActionFromHookMethod(syncVar, worker, hookMethod);
// set field
worker.Emit(OpCodes.Stfld, hookDelegate);
}

MethodDefinition FindHookMethod(TypeDefinition td, FieldDefinition syncVar, string hookFunctionName, ref bool WeavingFailed)
{
List<MethodDefinition> methods = td.GetMethods(hookFunctionName);
Expand Down Expand Up @@ -242,7 +261,7 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
// }
//
// the setter used to be manually IL generated, but we moved it to C# :)
public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition fd, string originalName, long dirtyBit, FieldDefinition netFieldId, ref bool WeavingFailed)
public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition fd, string originalName, long dirtyBit, FieldDefinition netFieldId, Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)> syncVarHookDelegates, ref bool WeavingFailed)
{
//Create the set method
MethodDefinition set = new MethodDefinition($"set_Network{originalName}", MethodAttributes.Public |
Expand Down Expand Up @@ -304,11 +323,17 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
// push the dirty bit for this SyncVar
worker.Emit(OpCodes.Ldc_I8, dirtyBit);

// hook? then push 'new Action<T,T>(Hook)' onto stack
// hook? then push 'this.HookDelegate' onto stack
MethodDefinition hookMethod = GetHookMethod(td, fd, ref WeavingFailed);
if (hookMethod != null)
{
GenerateNewActionFromHookMethod(fd, worker, hookMethod);
// Create the field that will store a single instance of the hook as a delegate (field will be set in constructor)
FieldDefinition hookActionDelegateField = CreateNewActionFieldDefinitionFromHookMethod(fd);
syncVarHookDelegates[fd] = (hookActionDelegateField, hookMethod);

// push this.hookActionDelegateField
worker.Emit(OpCodes.Ldarg_0);
worker.Emit(OpCodes.Ldfld, hookActionDelegateField);
}
// otherwise push 'null' as hook
else
Expand Down Expand Up @@ -362,7 +387,7 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
return set;
}

public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds, long dirtyBit, ref bool WeavingFailed)
public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds, Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)> syncVarHookDelegates, long dirtyBit, ref bool WeavingFailed)
{
string originalName = fd.Name;

Expand Down Expand Up @@ -391,7 +416,7 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
}

MethodDefinition get = GenerateSyncVarGetter(fd, originalName, netIdField);
MethodDefinition set = GenerateSyncVarSetter(td, fd, originalName, dirtyBit, netIdField, ref WeavingFailed);
MethodDefinition set = GenerateSyncVarSetter(td, fd, originalName, dirtyBit, netIdField, syncVarHookDelegates, ref WeavingFailed);

//NOTE: is property even needed? Could just use a setter function?
//create the property
Expand All @@ -417,10 +442,11 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
}
}

public (List<FieldDefinition> syncVars, Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds) ProcessSyncVars(TypeDefinition td, ref bool WeavingFailed)
public (List<FieldDefinition> syncVars, Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds, Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)> syncVarHookDelegates) ProcessSyncVars(TypeDefinition td, ref bool WeavingFailed)
{
List<FieldDefinition> syncVars = new List<FieldDefinition>();
Dictionary<FieldDefinition, FieldDefinition> syncVarNetIds = new Dictionary<FieldDefinition, FieldDefinition>();
Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)> syncVarHookDelegates = new Dictionary<FieldDefinition, (FieldDefinition hookDelegateField, MethodDefinition hookMethod)>();

// the mapping of dirtybits to sync-vars is implicit in the order of the fields here. this order is recorded in m_replacementProperties.
// start assigning syncvars at the place the base class stopped, if any
Expand Down Expand Up @@ -460,7 +486,7 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
{
syncVars.Add(fd);

ProcessSyncVar(td, fd, syncVarNetIds, 1L << dirtyBitCounter, ref WeavingFailed);
ProcessSyncVar(td, fd, syncVarNetIds, syncVarHookDelegates, 1L << dirtyBitCounter, ref WeavingFailed);
dirtyBitCounter += 1;

if (dirtyBitCounter > SyncVarLimit)
Expand All @@ -479,12 +505,18 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
td.Fields.Add(fd);
}

// add all of the new SyncVar Action<T,T> fields
foreach((FieldDefinition hookDelegateInstanceField, MethodDefinition) entry in syncVarHookDelegates.Values)
{
td.Fields.Add(entry.hookDelegateInstanceField);
}

// include parent class syncvars
// fixes: https://github.com/MirrorNetworking/Mirror/issues/3457
int parentSyncVarCount = syncVarAccessLists.GetSyncVarStart(td.BaseType.FullName);
syncVarAccessLists.SetNumSyncVars(td.FullName, parentSyncVarCount + syncVars.Count);

return (syncVars, syncVarNetIds);
return (syncVars, syncVarNetIds, syncVarHookDelegates);
}
}
}