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

Timeline for V8 fast-calls header exposed for native addons? #52923

Open
uNetworkingAB opened this issue May 9, 2024 · 8 comments
Open

Timeline for V8 fast-calls header exposed for native addons? #52923

uNetworkingAB opened this issue May 9, 2024 · 8 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@uNetworkingAB
Copy link

uNetworkingAB commented May 9, 2024

What is the problem this feature will solve?

Fast calls in V8 was introduced 4 years ago, soon to be half a decade ago. These features are still not exposed to native addon authors due to "it may change"-argumentation (see previous threads).

When can these features be freely used by the ecosystem as they see fit? Are we supposed to wait half a decade more? There clearly are many useful cases where it dramatically helps performance.

Why not just let the ecosystem adopt them as they see usable? We already have gone through 10+ years of V8 breaking API changes from version to version so none of this is news to any of us making native addons - V8 has never been stable in its API, not even the early wrappers were ever stable.

It always changed over time, so to treat fast calls differently than any other potential API change makes no sense. Esp. when it holds back performance innovation that technically already is available.

What is the feature you are proposing to solve the problem?

To include the v8-fast-api-calls.h so that native addons can use them.

What alternatives have you considered?

Hacking it in myself, on my own end

@uNetworkingAB uNetworkingAB added the feature request Issues that request new features to be added to Node.js. label May 9, 2024
@joyeecheung
Copy link
Member

I am pretty sure it's not distributed in the tarball simply because no-one remembered to add them to the list in

def wanted_v8_headers(options, files_arg, dest):
? But @targos may know more.

@targos
Copy link
Member

targos commented May 10, 2024

It was asked by the V8 team that we didn't expose it: #37570 (comment)

I don't object to add it if the situation is better now.

@joyeecheung
Copy link
Member

joyeecheung commented May 10, 2024

@benjamingr
Copy link
Member

benjamingr commented May 10, 2024

Hey Alex,

I believe the project is in a much more performance-oriented state than it was when you last engaged and there are more people like Yegiz and Daniel involved invested in improving performance related C++ stuff than before.

That said, this is still an alt account after your primary one was blocked. This whole saga is needlessly adversarial. We are happy to interact constructively towards a faster better Node.

I see your edits on the original issue. I recommend you:

  • Apologize for past behavior (almost a decade ago at this point) and state you won't violate the CoC and engage constructively.
  • Set up a meeting with the Node.js performance people like Yagiz and Daniel. I am sure you will be able to teach them a lot about the current bottlenecks and how to improve them. You will need to be patient as most of them don't have the context you do.

I believe you will be surprised at what can be accomplished at the current state quickly. Performance is getting a lot of love and things that got pushback in the past are a lot more likely to land with current project leadership.

In particular I think opting-out of async_hooks related overhead in MakeCallback and other "dangerous" changes from the past are on the table.


As for the actual change, V8 had 2 years to bake. Maya is no longer at Google IIRC, maybe @syg can assist with regards to if this is fine to expose?

@uNetworkingAB
Copy link
Author

Exposing this header would unlock major improvements to addon performance for several cases.

The volatility/instability argument makes no sense in my book, as I've already seen V8 APIs massively break, continuously, for 10 years. So why would it be any different this time? It wouldn't.

We all know V8 is an unstable API and always has been. Its part of the charm.

@uNetworkingAB
Copy link
Author

@joyeecheung Those are interesting reads but don't change the fact V8 has always been API-breaking. This measurement they did is interesting:

// M1
Start measuring noop_maybe_fast:
noop_maybe_fast: 103.16700000000003 ←—--
Start measuring noop_always_slow:
noop_always_slow: 299.08399999999995

A noop function is 3x the performance with fast calls vs. ordinary ones.

@joyeecheung
Copy link
Member

I don’t think there is need to explain the performance need here since that’s already used in Node.js. I would say it’s less up to Node.js but more up to the V8 team to gauge whether it’s fine to be included in the tarball now, as the maintenance burden of doing so is more on their side (there is another burden of ABI stability on Node.js’s side but AFAICT there are many other parts that make it difficult to upgrade V8 in a release line already).

@uNetworkingAB
Copy link
Author

Simply copying the singular v8-fast-api-calls.h for the major Node.js version into the repo worked and I now have precompiled binaries with fast API calls confirmed working.

Technically I could keep doing this, having each major version of v8-fast-api-calls.h per each major version of Node.js built for, but it would be a lot simpler if the header dist just included it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
Status: Pending Triage
Development

No branches or pull requests

4 participants