src: add V8 Fast API for URL.revokeObjectURL

[dario-piotrowicz]

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 🫶

[targos]

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]

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

[nodejs-github-bot]

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

[codecov]

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]

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

[BridgeAR]

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]

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]

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