doc: add v8 fast api contribution guidelines by anonrig · Pull Request #46199 · nodejs/node

anonrig · 2023-01-13T14:07:12Z

I'm introducing a guideline to define and share the limitations/caveats and experiences I've gained from researching on this topic. This would help new contributors to add fast APIs to existing functions.

Referencing: nodejs/performance#23

nodejs-github-bot · 2023-01-13T14:07:16Z

Review requested:

@nodejs/tsc

doc/contributing/adding-v8-fast-api.md

ronag

Instead of calculateX could you maybe have div(a, b) as an example and have divide by 0 as example for when to switch to slow path?

tniessen

cc @devsnek who has done most of the fast API work in node IIRC

tniessen · 2023-01-13T14:47:47Z

doc/contributing/adding-v8-fast-api.md

+Node.js uses [V8](https://github.com/v8/v8) as the JavaScript engine.
+In order to provide fast paths for functions that are called quite often,
+V8 proposes the usage of `fast api calls` that does not use any of the V8
+internals, but uses internal C functions.


internal C functions is rather ambiguous. Also, V8 internals usually refers to the non-public V8 API, which we try not to use in any case. Lastly, if I remember correctly, V8 API calls are allowed to some degree. Not performing any allocations on the V8 heap is what's critical. (Unless this changed since I last worked on fast API calls.)

I see. Thanks! Can you give an example of V8 API call, without any allocation on the V8 heap?

I might be wrong, but I think @MayaLekova implemented a fast API that relies on GetAlignedPointerFromInternalField in 6cfba9f, for example. @devsnek might know more about the exact conditions for which APIs are allowed.

You are right. You can access pre-existing/pre-allocated data from the fast apis. I thought it was inferred from the documentation (at least I assumed). Is there any specific wording you'd like me to change?

I've updated the documentation.

[...] Not performing any allocations on the V8 heap is what's critical.

Indeed that's still the case, and until we have the current GC architecture, it will stay like this. Possible future relaxation of this rule might come with conservative stack scanning. Please note that this also leads to the limitation for not calling back into JS code (I think this was discussed in this thread too).

Some brief background on the support for various types in V8 could be found in this external doc.

anonrig · 2023-01-13T15:16:09Z

Instead of calculateX could you maybe have div(a, b) as an example and have divide by 0 as example for when to switch to slow path?

I updated the example. Thanks for the hint! 🥇 💯

doc/contributing/adding-v8-fast-api.md

RafaelGSS

I think you should mention this file in the src/README.md too.

tony-go · 2023-01-13T21:28:25Z

Awesome @anonrig 🙌🏼

Learned a lot just by reading the PR ^^

doc/contributing/adding-v8-fast-api.md

nodejs-github-bot · 2023-01-20T03:25:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49099/

doc/contributing/adding-v8-fast-api.md

tniessen · 2023-01-20T09:59:27Z

doc/contributing/adding-v8-fast-api.md

+* Throwing errors is not available on fast API, but can be done
+  through the fallback to slow API.


Suggested change

* Throwing errors is not available on fast API, but can be done

through the fallback to slow API.

* Throwing errors is not available from within a fast API call, but can be done

through the fallback to the slow API.

Perhaps it's also better to suggest that since in principal, the fast API and the slow API should behave the same to avoid bugs, it's better not to throw errors in the slow API either and just update some flags and return to the JS land to throw?

I agree in principal with @joyeecheung, but that is not a limitation of a V8 Fast API declaration, but an implementation recommendation. I think it should not be included in this documentation, but in a more style specific documentation like README.md?

doc/contributing/adding-v8-fast-api.md

tniessen · 2023-01-20T10:08:02Z

doc/contributing/adding-v8-fast-api.md

+In V8, the options fallback is defined as `FastApiCallbackOptions` inside
+[`v8-fast-api-calls.h`](../../deps/v8/include/v8-fast-api-calls.h).
+
+* C++ land


Why is this a bullet point?

It aligns with other sections in the same file for mentioning the context of the example it's referring to. Do you have a recommendation for an alternative?

src/README.md

mcollina

lgtm

nodejs-github-bot · 2023-01-24T02:26:19Z

Commit Queue failed

- Loading data for nodejs/node/pull/46199
✔  Done loading data for nodejs/node/pull/46199
----------------------------------- PR info ------------------------------------
Title      doc: add v8 fast api contribution guidelines (#46199)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:docs-fast-api -> nodejs:main
Labels     doc, author ready, commit-queue-squash
Commits    7
 - doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
 - fixup! doc: add v8 fast api contribution guidelines
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/46199
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Darshan Sen 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46199
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Darshan Sen 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 13 Jan 2023 14:07:12 GMT
   ✔  Approvals: 5
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46199#pullrequestreview-1266448194
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/46199#pullrequestreview-1248744183
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/46199#pullrequestreview-1249572133
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/46199#pullrequestreview-1253500941
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/46199#pullrequestreview-1262907890
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-20T03:25:41Z: https://ci.nodejs.org/job/node-test-pull-request/49099/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - fixup! doc: add v8 fast api contribution guidelines
- Querying data for job/node-test-pull-request/49099/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3992434537

nodejs-github-bot · 2023-01-24T02:44:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49145/

nodejs-github-bot · 2023-01-24T03:46:04Z

Landed in 7dd4583

PR-URL: #46199 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 13, 2023

addaleax reviewed Jan 13, 2023

View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

anonrig force-pushed the docs-fast-api branch from 4944ab5 to dcc756f Compare January 13, 2023 14:13

ronag reviewed Jan 13, 2023

View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

anonrig force-pushed the docs-fast-api branch from dcc756f to 5a5669f Compare January 13, 2023 14:28

mcollina reviewed Jan 13, 2023

View reviewed changes

anonrig force-pushed the docs-fast-api branch from 5a5669f to a2251da Compare January 13, 2023 14:37

ronag reviewed Jan 13, 2023

View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

ronag reviewed Jan 13, 2023

View reviewed changes

anonrig force-pushed the docs-fast-api branch from a2251da to 177f200 Compare January 13, 2023 14:50

tniessen reviewed Jan 13, 2023

View reviewed changes

anonrig force-pushed the docs-fast-api branch from 177f200 to da9f97e Compare January 13, 2023 15:15

aduh95 reviewed Jan 13, 2023

View reviewed changes

VoltrexKeyva reviewed Jan 13, 2023

View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

RafaelGSS reviewed Jan 13, 2023

View reviewed changes

devsnek self-requested a review January 13, 2023 19:52

anonrig force-pushed the docs-fast-api branch 3 times, most recently from 90c81b8 to 66a8977 Compare January 13, 2023 21:57

anonrig requested review from RafaelGSS, VoltrexKeyva, addaleax, aduh95, mcollina, ronag and tniessen January 13, 2023 22:00

bnoordhuis reviewed Jan 14, 2023

View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

doc/contributing/adding-v8-fast-api.md Outdated Show resolved Hide resolved

doc/contributing/adding-v8-fast-api.md Show resolved Hide resolved

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2023

tniessen reviewed Jan 20, 2023

View reviewed changes

This was referenced Jan 21, 2023

CI Reliability 2023-01-21 nodejs/reliability#490

Open

CI Reliability 2023-01-22 nodejs/reliability#491

Open

CI Reliability 2023-01-23 nodejs/reliability#492

Open

mcollina approved these changes Jan 23, 2023

View reviewed changes

fixup! doc: add v8 fast api contribution guidelines

a2e2d67

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 23, 2023

github-actions bot mentioned this pull request Jan 24, 2023

CI Reliability 2023-01-24 nodejs/reliability#493

Open

13 tasks

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2023

anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 24, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2023

nodejs-github-bot merged commit 7dd4583 into nodejs:main Jan 24, 2023

anonrig deleted the docs-fast-api branch January 24, 2023 03:46

github-actions bot mentioned this pull request Jan 25, 2023

CI Reliability 2023-01-25 nodejs/reliability#494

Open

12 tasks

ruyadorno mentioned this pull request Feb 1, 2023

v19.6.0 proposal #46455

Merged

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

		* Throwing errors is not available on fast API, but can be done
		through the fallback to slow API.

Uh oh!

Conversation

anonrig commented Jan 13, 2023

Uh oh!

nodejs-github-bot commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

tony-go commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 24, 2023

ronag left a comment •

edited

Loading

anonrig Jan 13, 2023 •

edited

Loading