-
Notifications
You must be signed in to change notification settings - Fork 154
Upgrade to NET8 #112
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?
Upgrade to NET8 #112
Conversation
…o SDK-style Co-authored-by: bwinsley <64841770+bwinsley@users.noreply.github.com>
…lity Co-authored-by: bwinsley <64841770+bwinsley@users.noreply.github.com>
Co-authored-by: bwinsley <64841770+bwinsley@users.noreply.github.com>
Co-authored-by: bwinsley <64841770+bwinsley@users.noreply.github.com>
|
I'm not the maintainer of this project so I can't tell you if and when this could potentially be merged and won't review the concrete .NET 8 changes. But I did a lot of contributions in the past including performance work when we integrated this into our product, so I will at least comment on the things I noticed there. It's been a while so forgive inaccuracies, but I believe the change to To get the benefit of both caching the JS instances and allowing the GC of C# objects, #95 introduced a call to With all the lifetime and performance discussion above and in #95 I'm curious to know why the change to handles was |
| JavascriptExternal::JavascriptExternal(System::Object^ iObject) | ||
| { | ||
| mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject); | ||
| // .NET 8: Store GCHandle as IntPtr for safe native storage |
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.
Why the change to handles at all? What benefit do they have? (see comment on PR for more details)
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.
So it seemed that passing GCHandle to unmanaged c++ code would cause issues in NET8 and accessing methods on it such as GCHandle.Target would throw System.AccessViolationException. The solution was to use the method GCHandleToIntPtr to obtain a pointer which we can pass to unmanaged code
|
Apologies, I probably should have marked this as a draft first. I am working with @oliverbock and have probably opened this PR too early. Thanks for the look over though, I will have a closer consideration of what you've brought up |
|
Oh no worries at all! I had a bit of time and commented on what I noticed. If you're working on this together with Oliver then you have all the expertise and knowledge anyway and I will retreat back to the shadows 😄 And thank you for working on this! |
|
Thanks for the review! There was undoubtedly an issue with not cleaning up the dictionary as seen in the below images. The first image is before restoring the dictionary clean up and the second image is after restoring.
I noted that the memory was increasing linearly (or logarithmically?) unlike how you described the output to be in #95 and found there to be an accumulation of memory caused by JavascriptFunctions. I believe this is caused by the anonymous javascript function
|
|
IIRC the memory increase when using |
|
In other words: If you comment out the "Commit 5" scenario that calls the method with the callback in the test code and then see a flat curve, you should be back on par. |
…ild and run tests
| // Data members | ||
| //////////////////////////////////////////////////////////// | ||
| internal: | ||
| public: |
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.
Is this supposed to be public? It was internal before.
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.
We were previously using this member in our own module when it was protected but this has since become broken since it became internal. Changing it to public will keep it accessible by non inheriting classes and also by our own module.



This pull request modernizes the project by upgrading it from .NET Framework 4.7.2 to .NET 8, updating build configurations, and improving memory management for managed JavaScript function wrappers. It also revises documentation and sample code to reflect these changes, ensuring compatibility with current development tools and practices.
.NET 8 Migration and Build System Updates:
Fiddling.csprojandJavaScript.Net.vcxprojto .NET 8 SDK-style projects, removed legacy .NET Framework settings, and updated platform/toolset requirements for C++/CLI to useCLRSupport=NetCore. [1] [2]JavaScript.Net.slnfor Visual Studio 2022/2019 and removed references to older Visual Studio versions.app.configand legacy assembly references, as .NET 8 includes most system assemblies by default. [1] [2]Changes to method of tracking and garbage collecting JavascriptExternals
GCHandleto unmanagedc++code would cause issues in NET8 and accessing methods on it such asGCHandle.Targetwould throwSystem.AccessViolationException.GCHandle.ToIntPtrto obtain a pointer which we can pass to unmanaged codeMemory Management Enhancements:
JavascriptFunctionWrapperandWrappedJavascriptFunction) for managed JavaScript function objects, enabling proper cleanup of V8 handles and managed references when the context is disposed. Updated construction and disposal logic in both header and implementation files. [1] [2] [3] [4]Program.csinFidding.csproj[1] [2] [3]Before:

After:
This upgrade brings the project up to date with current .NET and Visual Studio standards, improves reliability and maintainability, and provides clearer guidance for users and contributors.