Skip to content

src: fix broken fast callback signatures #59055

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.

Sign up for GitHub

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

Jump to bottom
Closed
Renegade334 wants to merge 4 commits into from
Closed

src: fix broken fast callback signatures #59055

Renegade334 wants to merge 4 commits into from

Conversation

@Renegade334
Copy link
Member

@Renegade334 Renegade334 commented Jul 13, 2025

Since #54408, the various fast callbacks that were edited by that PR have broken signatures and have not been invoked. (The changed signatures would only have been valid if the receiver were prepended to the function arguments wherever those bindings were called from JS, which isn't the case.)

There was no debug tracking of these callbacks, so this went unnoticed at the time.

This change fixes the broken signatures and adds debug testing to validate the fast paths.

This effectively reverts the remaining source changes from #54408 that were not already covered by #58054.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 13, 2025

Review requested:

  • @nodejs/wasi

@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 Jul 13, 2025
@Renegade334 Renegade334 force-pushed the fix-histogram-fast-callbacks branch 3 times, most recently from 93048b8 to 9926389 Compare July 14, 2025 00:09
@codecov
Copy link

codecov bot commented Jul 14, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.10%. Comparing base (3b57443) to head (be6287b).
⚠️ Report is 197 commits behind head on main.

Files with missing lines Patch % Lines
src/histogram.cc 30.76% 8 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59055      +/-   ##
==========================================
+ Coverage   90.04%   90.10%   +0.06%     
==========================================
  Files         648      648              
  Lines      191078   191075       -3     
  Branches    37452    37457       +5     
==========================================
+ Hits       172050   172164     +114     
+ Misses      11649    11534     -115     
+ Partials     7379     7377       -2
Files with missing lines Coverage Δ
src/histogram.h 60.00% <ø> (ø)
src/node_process.h 100.00% <100.00%> (+75.00%) ⬆️
src/node_process_methods.cc 88.69% <100.00%> (+0.88%) ⬆️
src/node_wasi.cc 65.81% <ø> (+1.56%) ⬆️
src/node_wasi.h 0.00% <ø> (ø)
src/timers.cc 96.39% <100.00%> (+8.10%) ⬆️
src/timers.h 100.00% <ø> (ø)
src/histogram.cc 79.16% <30.76%> (+3.74%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@legendecas
Copy link
Member

legendecas commented Jul 18, 2025

It seems to me that #54408 should be reverted as a whole, rather than reverting partially, e.g. the documentations.

@legendecas legendecas mentioned this pull request Jul 18, 2025
Merged
@Renegade334
Copy link
Member Author

Renegade334 commented Jul 18, 2025

It seems to me that #54408 should be reverted as a whole, rather than reverting partially, e.g. the documentations.

This needs overhauling in its entirety anyway (#58999) and I have effectively reverted that aspect of the guidance there.

@legendecas
Copy link
Member

legendecas commented Jul 18, 2025 •
edited
Loading

The suggestion about passing receiver as the second argument at https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md#requirements is still there, should it be reverted as well? Otherwise the change in this PR is not aligning with the suggestion.

@Renegade334
Copy link
Member Author

Renegade334 commented Jul 18, 2025

It's removed in #58999, or are you referring to this PR?

nodejs-github-bot pushed a commit that referenced this pull request Jul 21, 2025
@legendecas
doc: avoid suggesting testing fast api with intense loop
450009d 
PR-URL: #59111
Refs: #59055
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
targos pushed a commit that referenced this pull request Jul 22, 2025
@legendecas @targos
doc: avoid suggesting testing fast api with intense loop
4ffa756 
PR-URL: #59111
Refs: #59055
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
@Renegade334 Renegade334 force-pushed the fix-histogram-fast-callbacks branch from 9926389 to be6287b Compare July 27, 2025 11:19
Renegade334
Renegade334 commented Jul 27, 2025
View reviewed changes
src/histogram.cc
Comment on lines -190 to -194
if (value < 1) {
HandleScope scope(options.isolate);
THROW_ERR_OUT_OF_RANGE(options.isolate, "value is out of range");
return;
}
Copy link
Member Author

@Renegade334 Renegade334 Jul 27, 2025

Choose a reason for hiding this comment

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

value is validated in the JS layer, so an assertion check should be fine here.

@Renegade334
Copy link
Member Author

Renegade334 commented Jul 27, 2025

cc @nodejs/cpp-reviewers – looking for reviews if possible.

aduh95 pushed a commit that referenced this pull request Aug 4, 2025
@legendecas @aduh95
doc: avoid suggesting testing fast api with intense loop
ba380f7 
PR-URL: #59111
Refs: #59055
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
@Renegade334 Renegade334 deleted the fix-histogram-fast-callbacks branch August 23, 2025 17:52
@Renegade334 Renegade334 mentioned this pull request Aug 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Renegade334 @nodejs-github-bot @legendecas