From e0cf8ae62a28bf78c5e956d2a0de10bb7a57d2bf Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 27 Apr 2025 10:52:29 -0400 Subject: [PATCH] url: improve canParse() performance for non-onebyte strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/58023 Reviewed-By: Daniel Lemire Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: James M Snell --- src/node_external_reference.h | 9 +++++ src/node_url.cc | 46 ++++++++++++++++++----- src/node_url.h | 13 +++++-- test/parallel/test-whatwg-url-canparse.js | 12 +++--- 4 files changed, 62 insertions(+), 18 deletions(-) diff --git a/src/node_external_reference.h b/src/node_external_reference.h index 1a1ba1dd5a5..c652c6878c3 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -10,6 +10,13 @@ namespace node { +using CFunctionCallbackWithalueAndOptions = bool (*)( + v8::Local, v8::Local, v8::FastApiCallbackOptions&); +using CFunctionCallbackWithMultipleValueAndOptions = + bool (*)(v8::Local, + v8::Local, + v8::Local, + v8::FastApiCallbackOptions&); using CFunctionCallbackWithOneByteString = uint32_t (*)(v8::Local, const v8::FastOneByteString&); @@ -92,6 +99,8 @@ class ExternalReferenceRegistry { #define ALLOWED_EXTERNAL_REFERENCE_TYPES(V) \ V(CFunctionCallback) \ + V(CFunctionCallbackWithalueAndOptions) \ + V(CFunctionCallbackWithMultipleValueAndOptions) \ V(CFunctionCallbackWithOneByteString) \ V(CFunctionCallbackReturnBool) \ V(CFunctionCallbackReturnDouble) \ diff --git a/src/node_url.cc b/src/node_url.cc index a233eea39b6..0a374f623fe 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -10,6 +10,7 @@ #include "path.h" #include "util-inl.h" #include "v8-fast-api-calls.h" +#include "v8-local-handle.h" #include "v8.h" #include @@ -21,7 +22,7 @@ namespace url { using v8::CFunction; using v8::Context; -using v8::FastOneByteString; +using v8::FastApiCallbackOptions; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; @@ -282,18 +283,45 @@ void BindingData::CanParse(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(can_parse); } -bool BindingData::FastCanParse(Local receiver, - const FastOneByteString& input) { +bool BindingData::FastCanParse( + Local receiver, + Local input, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + FastApiCallbackOptions& options) { TRACK_V8_FAST_API_CALL("url.canParse"); - return ada::can_parse(std::string_view(input.data, input.length)); + auto isolate = options.isolate; + HandleScope handleScope(isolate); + Local str; + if (!input->ToString(isolate->GetCurrentContext()).ToLocal(&str)) { + return false; + } + Utf8Value utf8(isolate, str); + return ada::can_parse(utf8.ToStringView()); } -bool BindingData::FastCanParseWithBase(Local receiver, - const FastOneByteString& input, - const FastOneByteString& base) { +bool BindingData::FastCanParseWithBase( + Local receiver, + Local input, + Local base, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + FastApiCallbackOptions& options) { TRACK_V8_FAST_API_CALL("url.canParse.withBase"); - auto base_view = std::string_view(base.data, base.length); - return ada::can_parse(std::string_view(input.data, input.length), &base_view); + auto isolate = options.isolate; + HandleScope handleScope(isolate); + auto context = isolate->GetCurrentContext(); + Local input_str; + if (!input->ToString(context).ToLocal(&input_str)) { + return false; + } + Local base_str; + if (!base->ToString(context).ToLocal(&base_str)) { + return false; + } + Utf8Value input_utf8(isolate, input_str); + Utf8Value base_utf8(isolate, base_str); + + auto base_view = base_utf8.ToStringView(); + return ada::can_parse(input_utf8.ToStringView(), &base_view); } CFunction BindingData::fast_can_parse_methods_[] = { diff --git a/src/node_url.h b/src/node_url.h index 74f8a49955c..c41dc930433 100644 --- a/src/node_url.h +++ b/src/node_url.h @@ -51,10 +51,15 @@ class BindingData : public SnapshotableObject { static void CanParse(const v8::FunctionCallbackInfo& args); static bool FastCanParse(v8::Local receiver, - const v8::FastOneByteString& input); - static bool FastCanParseWithBase(v8::Local receiver, - const v8::FastOneByteString& input, - const v8::FastOneByteString& base); + v8::Local input, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + v8::FastApiCallbackOptions& options); + static bool FastCanParseWithBase( + v8::Local receiver, + v8::Local input, + v8::Local base, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + v8::FastApiCallbackOptions& options); static void Format(const v8::FunctionCallbackInfo& args); static void GetOrigin(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-whatwg-url-canparse.js b/test/parallel/test-whatwg-url-canparse.js index 01195fb31b9..f3972a8b9c8 100644 --- a/test/parallel/test-whatwg-url-canparse.js +++ b/test/parallel/test-whatwg-url-canparse.js @@ -20,13 +20,17 @@ assert.throws(() => { assert.strictEqual(URL.canParse('https://example.org'), true); { - // V8 Fast API + // Only javascript methods can be optimized through %OptimizeFunctionOnNextCall + // This is why we surround the C++ method we want to optimize with a JS function. function testFastPaths() { // `canParse` binding has two overloads. assert.strictEqual(URL.canParse('https://www.example.com/path/?query=param#hash'), true); assert.strictEqual(URL.canParse('/', 'http://n'), true); } + // Since our JS function contains other javascript functions, + // we need to specify which function we want to optimize. This is why + // the next line does not optimize "testFastPaths" but "URL.canParse" eval('%PrepareFunctionForOptimization(URL.canParse)'); testFastPaths(); eval('%OptimizeFunctionOnNextCall(URL.canParse)'); @@ -34,9 +38,7 @@ assert.strictEqual(URL.canParse('https://example.org'), true); if (common.isDebug) { const { getV8FastApiCallCount } = internalBinding('debug'); - // TODO: the counts should be 1. The function is optimized, but the fast - // API is not called. - assert.strictEqual(getV8FastApiCallCount('url.canParse'), 0); - assert.strictEqual(getV8FastApiCallCount('url.canParse.withBase'), 0); + assert.strictEqual(getV8FastApiCallCount('url.canParse'), 1); + assert.strictEqual(getV8FastApiCallCount('url.canParse.withBase'), 1); } }