-
Notifications
You must be signed in to change notification settings - Fork 104
feat: improve garbage collection for injected types & implement finalizer support & stop leaking memory where we can avoid it. #216
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
base: master
Are you sure you want to change the base?
Conversation
|
Hello 👋 I don't normally work on runtime stuff, so I'm not sure I feel comfortable reviewing your pull request, but it did not go unnoticed. FYI I'm currently working on the v2 rewrite, which could cause issues for your pull request. I'm looking at getting an implementation for the generation changes pushed soon. |
|
exciting! could you point me to your work (if available online, no worries if not) so i can see how we intersect? |
Not yet available. I'm going to make a pull request when I push. |
|
ds5678
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I'm excited for your pull request and want to see it be part of v2.
| public IntPtr MethodInfo; | ||
| public Delegate ReferencedDelegate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very happy to see this. One of the things in v2 that was bothering me is that I wasn't sure what to do with these fields. I'm introducing a breaking change that injected classes can't have managed state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think breaking change is the only way to fix the semantics. i have a branch somewhere for bepinex which fixes it there.
| ClassInjector.DerivedConstructorBody(this); | ||
| } | ||
|
|
||
| private void Il2CppFinalize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v2 replaces [HideFromIl2Cpp] with an opt-in system. The user indicator for this would be:
[Il2CppMethod(Name = "Finalize")]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not exactly, i think the attribute does make sense for consistency but it would still need special-casing and the injected class' Finalize method would not directly translate to the one defined here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Name property applies in any situation where an Il2Cpp name differs from the generated name in managed code. In the case of finalizers, the v2 generator appends underscores, which is approximately the same as what you're doing.
| public Il2CppInterfaceCollection? Interfaces { get; init; } = null; | ||
| } | ||
|
|
||
| internal record FinalizerChain(Delegate finalizer, FinalizerChain? next) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should just use a primary constructor rather than be a record.
|
|
||
| namespace Il2CppInterop.Runtime.InteropTypes; | ||
|
|
||
| public class Il2CppObjectBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il2CppObjectBase is being removed in v2 in lieu of injecting code directly into Il2CppSystem.Object. As much code as possible should be implemented with extension methods or other helper classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's wrong with Il2CppObjectBase 😭? where will the new Il2CppSystem.Object be defined? will it still have behaviour i can configure (like the finalizer, and initialisation)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a real type.
Il2Cppmscorlib
Your capabilities should not change.
|
i'm a little confused on what the roadmap is with v2. is there an issue or something which lays it out? as a downstream user, i need to know if v2 is "coming soon" or not; are we at that stage of saying "nothing gets merged until v2 because that's top priority"? if so, i can rebase on top of your branch, but if not, i'm also willing to rebase yours on mine once this is merged. i want to make this as easy a merge as possible for you. |
There's a milestone, which includes most of the v2 changes.
Yes, I'm making a WIP pull request sometime this week.
Yes, that will likely be the next merge. 1.5.1 was released as the "last commit before the massively breaking changes of v2."
I still need to:
At that point, my branch will still need additional runtime changes, but it'll be safe for you to rebase your changes and make a pull request onto my branch. |
|
thank you so much. |
~Foo()) be the method which is overriden, should we instead make this somepublic override void Il2CppFinalize()dummy method instead? It likely would make our implementation burden a little easier, and maybe elide theSuppressFinalizetrickery, but would be an ergonomics hit.