FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: add V8 Fast API for `URL.revokeObjectURL` by dario-piotrowicz · Pull Request #58544 · nodejs/node · GitHub

/ node Public

src: add V8 Fast API for URL.revokeObjectURL - #58544

Closed
dario-piotrowicz wants to merge 5 commits into
nodejs:mainfrom
dario-piotrowicz:dario-nic/fast-revoke-object-url
ConversationCommits (5)ChecksFiles changed
Closed

src: add V8 Fast API for URL.revokeObjectURL#58544
dario-piotrowicz wants to merge 5 commits into
nodejs:mainfrom
dario-piotrowicz:dario-nic/fast-revoke-object-url

Conversation

dario-piotrowicz commented Jun 1, 2025

Copy link
Copy Markdown
Member

Benchmarking results:

(not a huge perf improvement 😓)

PS: I've done this alongside @npaun, thanks a lot Nicholas for working this out with me and your C++ tips/guidance 🫶

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 1, 2025
dario-piotrowicz force-pushed the dario-nic/fast-revoke-object-url branch from b6efadf to 6c550e4 Compare June 1, 2025 10:39
targos suggested changes Jun 1, 2025
View reviewed changes

targos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we should stop implementing C++ APIs twice. I don't mean to ban usage of V8 Fast API, but there should be only one implementation, that is called from both regular and fast API callbacks.
There have already been regressions because of multiple implementations that were not kept in sync.

dario-piotrowicz force-pushed the dario-nic/fast-revoke-object-url branch 2 times, most recently from 7c628fb to 659e3c0 Compare June 1, 2025 11:00
src: add V8 Fast API for URL.revokeObjectURL
77fd7b8 
co-authored-by: Nicholas Paun <npaun@cloudflare.com>
dario-piotrowicz force-pushed the dario-nic/fast-revoke-object-url branch from 659e3c0 to 77fd7b8 Compare June 1, 2025 11:03
dario-piotrowicz mentioned this pull request Jun 1, 2025
Closed

dario-piotrowicz commented Jun 1, 2025

Copy link
Copy Markdown
Member Author

I think we should stop implementing C++ APIs twice. I don't mean to ban usage of V8 Fast API, but there should be only one implementation, that is called from both regular and fast API callbacks. There have already been regressions because of multiple implementations that were not kept in sync.

Thanks so very much for the comment @targos 🙏

I totally agree with the suggestion 🙂 , does this look good? 🙂 🙏 e947f44

dario-piotrowicz requested a review from targos June 1, 2025 11:40
dario-piotrowicz added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jun 1, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2025

nodejs-github-bot commented Jun 1, 2025

Copy link
Copy Markdown
Collaborator

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

codecov Bot commented Jun 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (968e2f4) to head (3e936b3).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/node_blob.cc 85.71% 0 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58544      +/-   ##
==========================================
+ Coverage   90.21%   90.24%   +0.02%     
==========================================
  Files         635      635              
  Lines      187580   187603      +23     
  Branches    36853    36857       +4     
==========================================
+ Hits       169231   169300      +69     
+ Misses      11108    11057      -51     
- Partials     7241     7246       +5
Files with missing lines Coverage Δ
src/node_blob.h 37.50% <ø> (ø)
src/node_external_reference.h 100.00% <ø> (ø)
src/node_blob.cc 76.37% <85.71%> (+0.69%) ⬆️

... and 32 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.

nodejs-github-bot commented Jun 1, 2025

Copy link
Copy Markdown
Collaborator

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

dario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2025
BridgeAR requested changes Jun 1, 2025
View reviewed changes

BridgeAR left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I read the benchmark results correct it is a five percent regression?

This would align with other benchmarks with a couple of the "fast" API changes. They often turn out to be slower than before.

I am just requesting changes to verify that. If it's not an issue, please feel free to dismiss my request.
Could you add your benchmark to the code to run it on our machines?

dario-piotrowicz removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2025

dario-piotrowicz commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

If I read the benchmark results correct it is a five percent regression?

This would align with other benchmarks with a couple of the "fast" API changes. They often turn out to be slower than before.

I am just requesting changes to verify that. If it's not an issue, please feel free to dismiss my request. Could you add your benchmark to the code to run it on our machines?

darn it, you're totally right, sorry I was so fixated on the number that I missed which was the faster one.... I was sure to have seen at some point my implementation be faster than node... but I might be misremembering it 😖

Thanks so much for catching it @BridgeAR!!! 🫶 and so sorry for my mistake! 😓

I've re-run the hyperfine command multiple times and on my machine yeah I am consistently getting this sort of result 😓

At this point I am completely fine closing this PR, and open a new PR to either remove this todo comment:

node/src/node_blob.cc

Line 453 in 0e157b6

// TODO(@anonrig): Add V8 Fast API to the following function

or convert it to a comment explaining the the fast API doesn't help here

how does that sound @BridgeAR? @anonrig?

(or maybe my implementation is flawed?)

dario-piotrowicz mentioned this pull request Jun 7, 2025
Merged

dario-piotrowicz commented Jun 7, 2025

Copy link
Copy Markdown
Member Author

Closing in favour of #58614, thanks a lot again for spotting the regression @BridgeAR 🙏

dario-piotrowicz deleted the dario-nic/fast-revoke-object-url branch June 7, 2025 14:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR requested changes
jasnell jasnell approved these changes
benjamingr benjamingr approved these changes
anonrig anonrig approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

7 participants

Back | FazBrowse Home | New Git URL