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

Add benchmarks, GC handle tracking test #157

Merged
merged 3 commits into from
Oct 7, 2023
Merged

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Oct 4, 2023

  • Add more micro-benchmarks covering a variety of basic .NET <-> JS calls.
  • Add GC handle tracking: track just a handle count in release mode, and allocation stack in debug mode.
  • Add a test case that uses the handle-tracking to verify proper GC after constructing a .NET instance from JS, then releasing it.
  • Implement string array marshalling for napi_create_platform (to support --enable-gc option)
  • Add TryGetValueExternal() API, which returns null if the value is not an external. This fixes a bug in marshalling that arose when constructing a ManagedHost instance for benchmarks.

I added GC handle tracking because I was struggling with GC pressure during the many iterations that BenchmarkDotNet likes to run. First I thought there was a leak, but the test confirmed there is not. The problem is just that BenchmarkDotNet can run millions of iterations, which may each allocate and release objects, and JS GC does not keep up, so successive iterations get gradually slower. I couldn't figure out a way to explicitly run the JS GC during tests without distorting the benchmark results. (The test runner carefully manages the .NET GC, but doesn't know anything about JS.) So the workaround for now is to reduce the number of iterations.

@jasongin jasongin requested a review from vmoroz October 6, 2023 20:49
Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jasongin jasongin merged commit 0e23327 into main Oct 7, 2023
7 checks passed
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.

2 participants