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

Refactor native APIs #159

Merged
merged 6 commits into from
Nov 5, 2023
Merged

Refactor native APIs #159

merged 6 commits into from
Nov 5, 2023

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Oct 12, 2023

This refactor moves all native APIs (napi_* functions) from the JSNativeApi.Interop class to a JSRuntime abstract class. Then a specific runtime (NodejsRuntime) implements the abstract class via (delayed) binding to the native APIs, while also handling some low-level details like GC pinning.

This has a few benefits:

  • API calls can be overridden by tests, either for full or partial mocking of the runtime or for specific behaviors like error injection. I have not yet written new tests that take advantage of this, but I plan to use it to test object lifetime and threading errors.
  • API calls can be traced by injecting a wrapper around the runtime API, with zero cost when tracing is not enabled (the wrapper is not injected). I am already finding this VERY helpful for debugging and performance optimization. See the TracingJSRuntime class.
  • It could be possible to multiple runtimes in the same process, since the native API bindings are no longer static.

The drawback is this adds another level of indirection: every native API call is a virtual method invocation (along with a null check for delay-loading). I know there was a lot of work (and code) put into avoiding both of those costs. But now that we have micro-benchmarks, we can fairly accurately measure the impact of the changes... or lack of impact. From what I can see so far, there is no measurable performance impact: all of the benchmark results are consistently within the margin of error (~4%) before and after the changes. (Sometimes a bit slower, sometimes a bit faster.) I still need to do some more perf measurements on other OS's.

At this point all tests are passing, but some API areas are not yet refactored. And, I want to think more about how to initialize and reference the current runtime thread-static in relation to the current JSValueScope and JSRuntimeContext thread-statics.

@jasongin jasongin requested a review from vmoroz October 12, 2023 04:04
@vmoroz
Copy link
Member

vmoroz commented Oct 12, 2023

I like the overall direction of making the Node-API low implementation pluggable.
My concerns mostly are:

  • We should avoid extra memory allocations if we can. The new API replaces all uses of char spans with strings. I would rather see them to co-exist. One is more efficient, while another is more convenient. The char span based APIs can be virtual, while string-based can be convenience wrappers on top of them.
  • While using virtual classes and inheritance seems to be the simplest solution, the alternative could be just providing an alternative set of native function pointers. I do not know which one is better. I have seen that Meta's JSI being based on the interface provides very good flexibility in a number of cases.
  • In JSI for Node-API where I implemented the same flexible approach, I found that the biggest challenges were:
    • Tracking the current set of APIs across multiple threads. We must be very careful with it and provide the correct context. In some cases I chose eager function binding to avoid issues.
    • Dynamic loading if interconnected APIs and providing default implementation if they miss. Such APIs must be loaded as a group or defaulted as a group. E.g. loading/saving preprocessed byte code. It made me think if it is worth loading native APIs on demand. Maybe it is better to load them all at once and do not waste time on each call. In C++ I managed avoiding the runtime check by substituting the whole function. I wonder if we can do the same here.

@vmoroz
Copy link
Member

vmoroz commented Oct 12, 2023

Please note that current code uses the C++-like delay load stubs to avoid any runtime checks.
It would be nice if we can keep the same mechanism.
Checking pointer every time is a step backwards.
Alternatively, we can load all APIs eagerly upfront.

@jasongin
Copy link
Member Author

We should avoid extra memory allocations if we can. The new API replaces all uses of char spans with strings.

Not all: just function names, symbol names, and error messages, which are almost always going to already be string type in the C# code. The GetValueString* APIs still use Span<T>. But I would be OK with moving all string encoding back to the layer above this.

the alternative could be just providing an alternative set of native function pointers

I considered that, but chose the inheritance approach because:

  • Inheritance is easier and more flexible to work with, e.g. to override individual methods.
  • A virtual method call is not any more expensive than an indirect call through a pointer to a set of functions (because that is essentially what a v-table is).
  • It is more convenient to be able to override the functions at a slightly higher level, to work with Span<> and out parameters rather than raw pointers.

Maybe it is better to load them all at once and do not waste time on each call.

I was assuming that would impact startup time, but maybe it's not much. I can try to benchmark it.

@vmoroz
Copy link
Member

vmoroz commented Oct 12, 2023

We should avoid extra memory allocations if we can. The new API replaces all uses of char spans with strings.

Not all: just function names, symbol names, and error messages, which are almost always going to already be string type in the C# code. The GetValueString* APIs still use Span<T>. But I would be OK with moving all string encoding back to the layer above this.

I see. If it is only for errors, then it must be fine. For errors we will have much more overhead anyway.

the alternative could be just providing an alternative set of native function pointers

I considered that, but chose the inheritance approach because:

  • Inheritance is easier and more flexible to work with, e.g. to override individual methods.
  • A virtual method call is not any more expensive than an indirect call through a pointer to a set of functions (because that is essentially what a v-table is).
  • It is more convenient to be able to override the functions at a slightly higher level, to work with Span<> and out parameters rather than raw pointers.

It makes sense. Thanks!

Maybe it is better to load them all at once and do not waste time on each call.

I was assuming that would impact startup time, but maybe it's not much. I can try to benchmark it.

It would be interesting to see. Maybe the whole code could become much simpler.
Though, the code as it is already looks much simpler.

@jasongin jasongin force-pushed the dev/jasongin/runtime-interface branch from 704536d to 6552b17 Compare October 27, 2023 03:14
@jasongin
Copy link
Member Author

jasongin commented Oct 27, 2023

I just pushed a big update to this PR:

  • All native APIs are moved into the new JSRuntime abstraction. The JSNativeApi.Interop class is eliminated.
  • Optional tracing is implemented for all APIs.
    • Function names are tracked in JSCallbackDescriptor / JSPropertyDescriptor to enable better tracing.
  • A generic Import() helper method eliminates the if (func != null) checks before every call to an unmanaged delegate. The check still happens but it's only in the helper instead of repeated everywhere.
    • I benchmarked the alternative approach of pre-binding all of the APIs. It was a little bit slow: around a millisecond on the first call vs ~80 nanoseconds for subsequent calls. (And there was no measurable difference from removing the if check.) Since the motivation would have been mostly just fewer lines of code, I don't think it's worth it after the other Import() change achieved most of the same benefit.
  • Since the native API bindings are in a JSRuntime instance, rather than static, that instance needs to be maintained with context. It is attached to JSValueScope because many API calls already have a scope instance conveniently available. Some other APIs still need to get the runtime via the current (thread-local) scope.
    • I rewrote the JSValueScope constructor to accommodate passing in a JSRuntime instance, and also to add more consistency checks related to the relationships between different types of scopes.
  • I realized we can use [CallerArgumentExpression] by defining the attribute type for .NET 4.x. So it is used again in a few places.
  • The C# language version is updated to 12. My favorite addition is the simpler (JS-like) syntax for arrays: [a, b] instead of new[] { a, b }. (But I disabled some analyzer rules that now wanted to aggressively use this syntax in ways I didn't like.)

@jasongin jasongin marked this pull request as ready for review October 27, 2023 04:05
@jasongin jasongin merged commit 8907200 into main Nov 5, 2023
7 checks passed
@jasongin jasongin deleted the dev/jasongin/runtime-interface branch November 5, 2023 20:05
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