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

src,test: ensure that V8 fast APIs are called #54317

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

targos
Copy link
Member

@targos targos commented Aug 11, 2024

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.

@targos targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 11, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 11, 2024
@targos
Copy link
Member Author

targos commented Aug 11, 2024

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.

src/node_debug.h Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member

joyeecheung commented Aug 11, 2024

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 targos removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 11, 2024
@targos targos marked this pull request as ready for review August 11, 2024 14:04
@targos
Copy link
Member Author

targos commented Aug 11, 2024

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

@anonrig
Copy link
Member

anonrig commented Aug 11, 2024

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?

src/node_debug.h Outdated Show resolved Hide resolved
src/node_debug.h Outdated Show resolved Hide resolved
test/parallel/test-debug-v8-fast-api.js Show resolved Hide resolved
typings/internalBinding/debug.d.ts Show resolved Hide resolved
src/node_debug.cc Show resolved Hide resolved
Copy link

codecov bot commented Aug 11, 2024

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

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 in thread-local storage and
an internal API can be called to get the total count associated with
a call id.
Specific tests are added and `crypto.timingSafeEqual` as well as
internal documentation are updated to show how to use the macro
and test fast API calls without running long loops.
@targos
Copy link
Member Author

targos commented Aug 11, 2024

@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.

src/node_debug.cc Outdated Show resolved Hide resolved
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024
@nodejs-github-bot

This comment was marked as outdated.

@targos targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos targos added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 12, 2024
thread_local std::unordered_map<std::string_view, int> v8_fast_api_call_counts;

void TrackV8FastApiCall(std::string_view key) {
v8_fast_api_call_counts[key]++;
Copy link
Member

@joyeecheung joyeecheung Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 13, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1d35a06 into nodejs:main Aug 13, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1d35a06

targos added a commit that referenced this pull request Aug 14, 2024
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 in thread-local storage and
an internal API can be called to get the total count associated with
a call id.
Specific tests are added and `crypto.timingSafeEqual` as well as
internal documentation are updated to show how to use the macro
and test fast API calls without running long loops.

PR-URL: #54317
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos added a commit that referenced this pull request Aug 14, 2024
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 in thread-local storage and
an internal API can be called to get the total count associated with
a call id.
Specific tests are added and `crypto.timingSafeEqual` as well as
internal documentation are updated to show how to use the macro
and test fast API calls without running long loops.

PR-URL: #54317
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants