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.

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

Conversation

cshunsinger
Copy link
Contributor

@cshunsinger cshunsinger commented Sep 21, 2023

This commit makes a couple of small modification to the weaving around syncvars.

I identified 2 cases in which we weave code to invoke the hook for setting a syncvar's value:

  1. When the setter is called on the weaved Network property
  2. When deserializing messages where the message contains a new value for the syncvar from the remote source

As stated in the linked issue, when the syncvar hook is invoked (if there even is a syncvar hook), an instantiation occurs:
new Action<T, T>(HookMethod)

The modifications to the weaving do the following:

  • For each syncvar that contains a hook, a new Action<T, T> instance field is added to the class being weaved, where T is the field type of the syncvar field.
  • During constructor injection weaving, all of the Action<T, T> instance fields are instantiated to new Action<T, T>(HookMethod) for each of the syncvar's hooks
  • In both cases where the hook is invoked, instead of pushing new Action<T, T>(HookMethod) to the stack, push this.hookActionDelegate to the stack.

ILSpy showing the weaving results: generated delegate fields, no more instantiations, and setting up the delegates in the constructor
image

image

image

resolves #3594

@cshunsinger cshunsinger changed the title SyncVar hook invocations no longer instantiate a new action delegate on every call perf: SyncVar hook invocations no longer instantiate a new action delegate on every call Sep 21, 2023
@miwarnec
Copy link
Collaborator

interesting, we'll take a look soon after high priority stuff

@James-Frowen
Copy link
Contributor

This is the commit that caused this performance regression: 5369b8f

the new action adds up fast, In a few projects I've seen it causes 100kb+ of allocations per frame...

@Liam2349
Copy link

Merged this in, no more GC. Thanks! Hopefully I didn't break anything.

@MrGadget1024 MrGadget1024 added bug Something isn't working Awaiting Review labels Nov 23, 2023
@miwarnec
Copy link
Collaborator

miwarnec commented Dec 6, 2023

we are still discussing this.
the original motivation was to move more and more IL code into C# to reduce weaving headaches.
and now it looks like Unity is going to deprecate weaving entirely: #3630

@miwarnec miwarnec merged commit 608429c into MirrorNetworking:master Dec 6, 2023
@miwarnec
Copy link
Collaborator

miwarnec commented Dec 6, 2023

@cshunsinger merged!
Not many people understand Weaver & IL code, so nice work!

@cshunsinger
Copy link
Contributor Author

@cshunsinger merged! Not many people understand Weaver & IL code, so nice work!

Let me know if you have any other weaving needs. I love navigating the low-level stuff whether its IL/bytecode, or other funky stuff.

With Unity deprecating weaving, that may get tricky. I wonder if the older versions of Unity all support Roslyn because if not then this project will end up being forced to support both Roslyn and IL weaving depending on which version of Unity is used O.O

@James-Frowen
Copy link
Contributor

With Unity deprecating weaving, that may get tricky. I wonder if the older versions of Unity all support Roslyn because if not then this project will end up being forced to support both Roslyn and IL weaving depending on which version of Unity is used O.O

Unity is deprecating the AssemblyBuilder class (which compiles .cs files), not IL weaving (which edits dlls).

IL weaving is done to the dll using mono.cecil. Currently unity has ILPostProcessor as an undocumented feature that lets us hook in and run it easily. but even if unity removes or replaces the current ILPostProcessor we can just go back to how Weaver worked in unity 2019 before ILPostProcessor existed.

@cshunsinger cshunsinger deleted the SyncVar-hook-use-cached-delegates branch December 8, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syncvar hooks allocate ~100kb per frame
6 participants