Skip to content

Conversation

@bwinsley
Copy link

@bwinsley bwinsley commented Nov 19, 2025

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:

  • Migrated Fiddling.csproj and JavaScript.Net.vcxproj to .NET 8 SDK-style projects, removed legacy .NET Framework settings, and updated platform/toolset requirements for C++/CLI to use CLRSupport=NetCore. [1] [2]
  • Updated solution file JavaScript.Net.sln for Visual Studio 2022/2019 and removed references to older Visual Studio versions.
  • Removed obsolete configuration file app.config and legacy assembly references, as .NET 8 includes most system assemblies by default. [1] [2]

Changes to method of tracking and garbage collecting JavascriptExternals

  • 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 GCHandle.ToIntPtr to obtain a pointer which we can pass to unmanaged code

Memory Management Enhancements:

  • Added a dedicated wrapper (JavascriptFunctionWrapper and WrappedJavascriptFunction) 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]
  • Readded memory management code into Program.cs in Fidding.csproj [1] [2] [3]
  • This change mimics that of Reduce memory inefficiencies and leaks #95
  • See performance improvement below

Before: image
After: image

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.

@spahnke
Copy link
Collaborator

spahnke commented Nov 19, 2025

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 JavascriptExternal regresses the biggest chunk
of memory improvements done in #95 (see details for "Commit 1"). A JavascriptExternal object is alive for the entire
duration of the engine (JavascriptContext/isolate) lifetime and wraps a concrete instance of a C# object. It is created
in JavascriptContext::WrapObject, never gets passed to the outside world so no one else can have references to it, and
is deleted in the destructor of JavascriptContext by iterating all mExternals. This dictionary is used to cache the
creation of JS instances that mirror the C# object (done in JavascriptInterop::WrapObject by the call to ToLocal) if
the same object reference from C# land is used over and over. This caching, however, means that the C# object can never
be GC'd until the JavascriptContext is destroyed. But this is a huge problem for long running/pooled engine instances
which e.g. I need to do because the creation of a JavascriptContext per JS execution is completely infeasible.

To get the benefit of both caching the JS instances and allowing the GC of C# objects, #95 introduced a call to SetWeak
in JavascriptExternal so that V8 can tell us if the instance is no longer in use by the engine (e.g. after one script
has executed completely, or during execution when no one holds a JS reference anymore). In that case GCCallback runs
and we not only delete the external itself freeing the GCHandle, but also remove the C# object reference from the mExternals dictionary.
Only this way Javascript.NET does not hold any references to the C# object anymore. This PR removed that cleanup and
therefore regressed that case making objects GC'able only after engine destruction.

With all the lifetime and performance discussion above and in #95 I'm curious to know why the change to handles was
made, and why the dictionary cleanup in GCCallback was removed. The commit only states "Fix garbage collection
issues".

JavascriptExternal::JavascriptExternal(System::Object^ iObject)
{
mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject);
// .NET 8: Store GCHandle as IntPtr for safe native storage
Copy link
Collaborator

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)

Copy link
Author

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

@bwinsley
Copy link
Author

@spahnke

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

@spahnke
Copy link
Collaborator

spahnke commented Nov 19, 2025

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!

@bwinsley bwinsley marked this pull request as draft November 20, 2025 02:01
@bwinsley
Copy link
Author

@spahnke

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.

image image

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 sum += product.GetSalesTax(p => p * 0.19). I'm looking into a solution that mimics your original memory inefficiency clean up for JavascriptExternals and am seeing better results now but I wonder why this became a problem in the first place as your results didn't seem to show it earlier.

image

@spahnke
Copy link
Collaborator

spahnke commented Nov 20, 2025

IIRC the memory increase when using JavascriptFunction was still noticeable for these test cases even after all the commits in #95 and the flat curve was when not using them. I believe that is what I mentioned under the "Commit 5" and "Conclusion" headings. Though that was never a big problem for us in practice as long as you always explicitly call Dispose on any obtained JavascriptFunction object in your usage code, which that commit made possible. I hope my memory doesn't fail me here.

@spahnke
Copy link
Collaborator

spahnke commented Nov 20, 2025

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.

@bwinsley bwinsley marked this pull request as ready for review November 21, 2025 03:19
// Data members
////////////////////////////////////////////////////////////
internal:
public:
Copy link
Collaborator

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants