Skip to content

url: speed up URLPattern - #65364

Open
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:cursor/urlpattern-performance-603e
Open

url: speed up URLPattern#65364
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:cursor/urlpattern-performance-603e

Conversation

@anonrig

@anonrig anonrig commented Aug 18, 2026

Copy link
Copy Markdown
Member

This speeds up WHATWG URLPattern constructor, test(), and exec() without changing observable behavior.

Independent of the new URL() parse PR (#65361) and the URLSearchParams PR (#65363).

What changed

C++ only (src/node_url_pattern.cc):

  • String input: copy one-byte ASCII V8 strings with ValueView + memcpy into a stack buffer and pass a string_view into Ada. Drop the extra std::string copy that BufferValue/Utf8Value::ToString() used to make. Non-ASCII still goes through Utf8Value.
  • Object init: look up the nine interned component keys and assign fields by index. Skip UTF-8-converting each key name and the string-compare chain. Non-string members are still ignored.
  • Regexp provider: convert pattern/input strings with ToV8Value and use isolate->GetCurrentContext() instead of Environment::GetCurrent.
  • test / exec: share argument parsing so both paths keep the same WebIDL null/undefined/"null" baseURL behavior.

Ada still owns its parsed state; string_views only live for the duration of the C++ call. ValueView is destroyed before any V8 heap allocation.

Tests

  • test/parallel/test-urlpattern.js, test-urlpattern-types.js, test-urlpattern-invalidthis.js, and test-urlpattern-fast-path.js all pass
  • WPT test/wpt/test-urlpattern.js: 743 passed, 0 unexpected failures

Review follow-up

  • Dropped the local NewStringFromUtf8View helper in favor of ToV8Value
  • CopyV8StringToBuffer takes a pointer, matching project convention for mutating arguments
  • Author and Signed-off-by are Yagiz Nizipli (human DCO)

Assisted-by: Cursor

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot 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. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 18, 2026

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

AI agents are not permitted to use Signed-off-by

Comment thread src/node_url_pattern.cc Outdated
length);
}
return String::NewFromUtf8(
isolate, view.data(), NewStringType::kNormal, length);

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.

We already have ToV8Value that takes an std::string_view

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switched these call sites to ToV8Value and dropped the local helper.

Comment thread src/node_url_pattern.cc Outdated
// and destroy the view before creating any V8 heap objects.
void CopyV8StringToBuffer(Isolate* isolate,
Local<String> str,
MaybeStackBuffer<char>& buffer) {

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.

Project convention is to pass pointers when mutating and to only pass refs as const ref

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

CopyV8StringToBuffer now takes MaybeStackBuffer<char>*.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.15044% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (f83e7df) to head (1ba9273).
⚠️ Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
src/node_url_pattern.cc 91.15% 1 Missing and 9 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65364      +/-   ##
==========================================
- Coverage   90.31%   90.13%   -0.18%     
==========================================
  Files         751      752       +1     
  Lines      249956   251861    +1905     
  Branches    47204    47350     +146     
==========================================
+ Hits       225745   227022    +1277     
- Misses      15612    16177     +565     
- Partials     8599     8662      +63     
Files with missing lines Coverage Δ
src/node_url_pattern.cc 83.84% <91.15%> (+0.12%) ⬆️

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

@cursor
cursor Bot force-pushed the cursor/urlpattern-performance-603e branch from 351c14d to 52a2a71 Compare August 18, 2026 11:39
@anonrig

anonrig commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

Removed the Signed-off-by trailer from the commit(s). An AI agent cannot attest the DCO.

@cursor
cursor Bot force-pushed the cursor/urlpattern-performance-603e branch from 52a2a71 to a2e81de Compare August 18, 2026 12:49
Avoid extra UTF-8 copies on constructor, test, and exec; convert
regexp inputs with ToV8Value; and assign URLPatternInit fields by
interned key instead of string-comparing each component name.

Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Assisted-by: Cursor
@cursor
cursor Bot force-pushed the cursor/urlpattern-performance-603e branch 5 times, most recently from ed512d0 to 1ba9273 Compare August 18, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants