src: introduce V8 fast api to guessHandleType

[anonrig]

Divided the changes into 2 commits:

• First commit returns uint32_t instead of string to remove unnecessary serialization.
• Second commit adds V8 Fast API.

[nodejs-github-bot]

Review requested:

• @nodejs/net
• @nodejs/startup

[RaisinTen]

Were you able to measure the performance improvement?

[anonrig]

Were you able to measure the performance improvement?

Unfortunately, I don't have the time, but imho the performance gains would be around 40-50% due to fast api.

[mscdex]

I can't imagine there being a measurable difference with these changes.

[anonrig]

I can't imagine there being a measurable difference with these changes.

I didn't understand. (English as a second language has its limitations): You don't believe that these changes would improve the performance. Right? Why would you think that?

[mscdex]

Why would you think that?

Because it's generally not something that would be done/called often enough and/or compared to other operations (e.g. performing I/O).

[RaisinTen]

If this function is not called very frequently, V8 might not prefer the fast API over the slow one and so, the new code might not even get run.

Given that this also hampers readability with practically very little performance gain (because it's not a hot code path AFAICT), is it worth making this change?

[mcollina]

LGTM

[nodejs-github-bot]

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

[cjihrig]

I'm curious about the second commit in this PR. Based on the comments and reactions, I'm guessing there are at least three people that question whether it should land at all since it complicates the code and is unlikely to show any real world improvements. Yet, there are claims of a possible 40-50% performance improvement with no benchmarks to confirm.

I don't think we should get in the habit of merging things like that. I also don't think we should add V8 fast paths everywhere possible, unless we know they are going to help (I'm pointing this out because similar suggestions have been made in other PRs recently).

[anonrig]

I'm curious about the second commit in this PR. Based on the comments and reactions, I'm guessing there are at least three people that question whether it should land at all since it complicates the code and is unlikely to show any real world improvements. Yet, there are claims of a possible 40-50% performance improvement with no benchmarks to confirm.

This claim was mostly an educated guess referencing the improvements in previous V8 Fast API implementations. Due to the lack of time to add benchmarks to support my claim, I've changed the pull request title. If you think it's worth to split the commits into separate pull requests, I can do that as well, but not as soon as one can expect.

I also don't think we should add V8 fast paths everywhere possible, unless we know they are going to help.

I understand your concern, but this forces to find a unique path to trigger V8 to optimize and call Fast API, and it is not always easy to find that path. I personally, don't see any downsides of adding V8 Fast API integrations, since it does not effect the rest of the code. What I personally also like about V8 Fast API is that the limitations of it, forces us to keep our API surface as minimal as possible. For example, due to limitations, we can't return strings, which is a good thing because we also need to avoid strings serialization.

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/48349
✔  Done loading data for nodejs/node/pull/48349
----------------------------------- PR info ------------------------------------
Title      src: introduce V8 fast api to `guessHandleType` (#48349)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:improve-guessHandleType -> nodejs:main
Labels     lib / src, performance, needs-ci, commit-queue-rebase
Commits    2
 - src: return uint32 for `guessHandleType`
 - src: add V8 fast api to `guessHandleType`
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/48349
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Minwoo Jung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48349
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: Minwoo Jung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - src: return uint32 for `guessHandleType`
   ⚠  - src: add V8 fast api to `guessHandleType`
   ℹ  This PR was created on Mon, 05 Jun 2023 16:41:28 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/48349#pullrequestreview-1465345051
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/48349#pullrequestreview-1465728186
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/48349#pullrequestreview-1471073777
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-06-14T04:03:31Z: https://ci.nodejs.org/job/node-test-pull-request/52248/
- Querying data for job/node-test-pull-request/52248/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

[nodejs-github-bot]

Landed in f3ee4e2...d2dfdd6

[ruyadorno]

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it to land in v18.