src: add a fast call for URL.revokeObjectURL

[debadree25]

This is an attempt at resolving a TODO left in #47728 regarding adding a fast call for URL.revokeObjectURL Unfortunately it seems like either the fast call is not getting called or there is no perf benefit for doing this most probably I have made the integration wrong but I am unable to understand what may be wrong hence opening this as a draft for any help possible

Thank You!

Refs: #47728

[debadree25]

cc @anonrig since you have experience with Fast Calls and left this todo 😅

[debadree25]

The results of running the benchmark in the pull req

                                             confidence improvement accuracy (*)   (**)  (***)
url/url-create-revoke-objecturl.js n=1000000                -3.27 %       ±3.31% ±4.41% ±5.74%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 1 comparisons, you can thus expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

[anonrig]

I couldn't find why the fast path is not triggered. Maybe @joyeecheung, @devsnek or @addaleax might help?

[anonrig]

@joyeecheung You used Local<v8::Object> for the receiver, whereas, in CanParse @KhafraDev used Local<v8::Value> for the receiver. Which one is the correct usage for the Fast API, or is both of them correct?

[joyeecheung]

The strings returned by createObjectURL() are cons strings. They won't hit the fast API.

[anonrig]

Is this documented? How can we update the benchmark to flatten the string?

(For people who didn't know what cons strings mean: cons strings are pairs of strings, result of concatenation.)

[anonrig]

Adding hello-1-2-3 as an input triggers fast api, but even crypto.randomUUID() as a parameter does not trigger fast API...

function main({ n }) {
  bench.start();
  for (let i = 0; i < n; i += 1) {
    URL.revokeObjectURL('hello-1-2-3');
  }
  bench.end(n);
}

[joyeecheung]

FastOneByteString means the string type is a sequential one byte string. The string types are internal to V8 and subject to changes. I don't think it's meaningful to flatten the string in the benchmark if users in the wild are going to pass urls returned by createObjectURL() to it...

[joyeecheung]

The JS-land version buffers the UUID so to match the performance the C++ version needs to buffer the UUID as well. Also snprintf() can be slow if it's heavily used.

[debadree25]

I think doing all this would probably make the C++ side way more complicated than the original TODO intended, is there no faster way of string allocation on C++ side?

[joyeecheung]

You could also just merge the concatenation in createObjectURL() from JS land into Blob::StoreDataObject and concatenate the strings from C++ instead.

[joyeecheung]

The string should be created with NewFromOneByte(), not NewFromUtf8() as in 268fc13 because it's guaranteed to be one-byte and does not need transcoding.

[debadree25]

okay so i tried this

diff --git a/src/node_blob.cc b/src/node_blob.cc
index 7db8684904..45242e7c44 100644
--- a/src/node_blob.cc
+++ b/src/node_blob.cc
@@ -391,13 +391,19 @@ void Blob::StoreDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   size_t length = args[2].As<Uint32>()->Value();
   Utf8Value type(env->isolate(), args[3]);
+  std::string key_str(*key, key.length());
 
   binding_data->store_data_object(
-      std::string(*key, key.length()),
-      BlobBindingData::StoredDataObject(
-        BaseObjectPtr<Blob>(blob),
-        length,
-        std::string(*type, type.length())));
+      key_str,
+      BlobBindingData::StoredDataObject(BaseObjectPtr<Blob>(blob),
+                                        length,
+                                        std::string(*type, type.length())));
+  std::string final_url = "blob:nodedata:" + key_str;
+  args.GetReturnValue().Set(String::NewFromOneByte(env->isolate(),
+                                                reinterpret_cast<const uint8_t*>(final_url.data()),
+                                                v8::NewStringType::kNormal,
+                                                final_url.length())
+                                .ToLocalChecked());
 }

fast paths are being hit and perf is maybe little bit better than 268fc13 but on the benchmark in the commit, its still slower than the normal js version unfortunately 😓