src,test: ensure that V8 fast APIs are called

[targos]

Adds a debug-only macro that can be used to track when a V8 fast API is
called. A map of counters is maintained in the Node.js environment and
an internal API can be called to get the total count associated with
a call id.
Specific tests are added and crypto.timingSafeEqual is updated to
show how to use the macro and test fast API calls without running long
loops.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp

[targos]

Context: I have a working implementation for V8 12.8 in targos@b6a84ac.

Unfortunately, the V8 version in main doesn't provide the isolate in FastApiCallbackOptions.
I need the isolate to access the Node.js environment (if there's another way, please tell me), so as a workaround, I'm getting the isolate from the receiver. But then it seems that there is no execution context. The tests crash because Environment::GetCurrent(isolate) returns a null pointer.

[joyeecheung]

You don’t have to put this in the Environment or make it associated with an isolate at all - using a thread_local map should already be enough.

[targos]

Thanks @joyeecheung. I changed it to use a thread_local variable.

[anonrig]

My only concern is that while working on anything related to performance, I always build node.js in release mode (not in debug). hence, the need to build performance pull-requests in different release modes, might increase the barrier and friction to contribute. Is there a possibility of enabling these checks by default but somehow the official release builds doesn't contain the output of this macro? or, can we introduce a new build flag to enable this on release builds without having to additionally build debug?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (298ff4f) to head (3201cf9).
Report is 436 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54317      +/-   ##
==========================================
- Coverage   87.09%   87.09%   -0.01%     
==========================================
  Files         647      647              
  Lines      181816   181843      +27     
  Branches    34884    34890       +6     
==========================================
+ Hits       158360   158376      +16     
- Misses      16764    16771       +7     
- Partials     6692     6696       +4     

Files with missing lines	Coverage Δ
src/crypto/crypto_timing.cc	80.00% <ø> (ø)
src/node_binding.cc	83.66% <ø> (+0.79%)	⬆️

... and 29 files with indirect coverage changes

[targos]

@anonrig I tried to address all your review comments.

Is there a possibility of enabling these checks by default but somehow the official release builds doesn't contain the output of this macro? or, can we introduce a new build flag to enable this on release builds without having to additionally build debug?

Since the macros are used in performance-sensitive paths, I wanted them to have 0 impact in release mode, so that's why I chose to use debug mode as the condition.
Note that this also works under the --debug-node compile-time flag, which doesn't require you to rebuild V8 and other deps. I think it's a good compromise, and not more of a contribution barrier than an entirely new and separate flag.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[joyeecheung]

For the purpose of the tests, I think using a bool should be enough - an int would overflow if the process runs for long enough.

Also if we require the counter/toggle to be pre-declared in a macro list, we can just track them in thread_local bool instead of doing a runtime map lookup, and the overhead of simply setting a variable to true feels negligible enough to be enabled at runtime.

[targos]

an int would overflow if the process runs for long enough

I know that but I don't think it matters. As an internal debug-only API, it is not meant to be useful outside of our unit tests and those would never run for long enough.

I thought a counter could be useful for complex cases where we want to call the same API multiple times (or it's called in a loop internally).

I'd say feel free to refactor / change in a future PR (or block this one). I spent enough energy on it myself.

[joyeecheung]

I thought a counter could be useful for complex cases where we want to call the same API multiple times (or it's called in a loop internally).

Even if it's called multiple times, only V8 can really reliably check exactly how many times the fast API is run, as that's subject to the optimizing strategy - unless we use the intrinsics that are not supported for embedders' use cases. As an embedder a true/false is the best we can count on.

I'd say feel free to refactor / change in a future PR (or block this one). I spent enough energy on it myself.

Sorry if this is untimely - feel free to land it, just trying to address the previous comments about release/debug differences.

[nodejs-github-bot]

Landed in 1d35a06