url: speed up URLPattern - #65364
Conversation
|
Review requested:
|
jasnell
left a comment
There was a problem hiding this comment.
AI agents are not permitted to use Signed-off-by
| length); | ||
| } | ||
| return String::NewFromUtf8( | ||
| isolate, view.data(), NewStringType::kNormal, length); |
There was a problem hiding this comment.
We already have ToV8Value that takes an std::string_view
There was a problem hiding this comment.
Switched these call sites to ToV8Value and dropped the local helper.
| // and destroy the view before creating any V8 heap objects. | ||
| void CopyV8StringToBuffer(Isolate* isolate, | ||
| Local<String> str, | ||
| MaybeStackBuffer<char>& buffer) { |
There was a problem hiding this comment.
Project convention is to pass pointers when mutating and to only pass refs as const ref
There was a problem hiding this comment.
CopyV8StringToBuffer now takes MaybeStackBuffer<char>*.
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
351c14d to
52a2a71
Compare
|
Removed the |
52a2a71 to
a2e81de
Compare
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
ed512d0 to
1ba9273
Compare
| } else if (key == "baseURL") { | ||
| init.base_url = std::string(value); | ||
| } | ||
| std::optional<std::string>* fields[] = { |
There was a problem hiding this comment.
@anonrig I don't have my AI with me, but this seems overly complicated.
The structure is just made of...
std::optional<std::string> protocol{};
// If present, must be valid UTF-8.
std::optional<std::string> username{};
// If present, must be valid UTF-8.
std::optional<std::string> password{};
// If present, must be valid UTF-8.
std::optional<std::string> hostname{};
// If present, must be valid UTF-8.
std::optional<std::string> port{};
// If present, must be valid UTF-8.
std::optional<std::string> pathname{};
// If present, must be valid UTF-8.
std::optional<std::string> search{};
// If present, must be valid UTF-8.
std::optional<std::string> hash{};
// If present, must be valid UTF-8.
std::optional<std::string> base_url{};The standard will lay them out one after the other without padding, I expect. So you can do (&init.protocol + index) to get the desired pointer, without materializing this fields array.
My idea won't impact the performance, probably, but it would require less code.
Of course, an even better option would be to
std::span<std::optional<std::string>> as_span() { return {&protocol, 9}; }to the init structure... and then you could do as_span()[i] and that would be that.
This speeds up WHATWG
URLPatternconstructor,test(), andexec()without changing observable behavior.Independent of the
new URL()parse PR (#65361) and theURLSearchParamsPR (#65363).What changed
C++ only (
src/node_url_pattern.cc):ValueView+memcpyinto a stack buffer and pass astring_viewinto Ada. Drop the extrastd::stringcopy thatBufferValue/Utf8Value::ToString()used to make. Non-ASCII still goes throughUtf8Value.ToV8Valueand useisolate->GetCurrentContext()instead ofEnvironment::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.ValueViewis destroyed before any V8 heap allocation.Tests
test/parallel/test-urlpattern.js,test-urlpattern-types.js,test-urlpattern-invalidthis.js, andtest-urlpattern-fast-path.jsall passtest/wpt/test-urlpattern.js: 743 passed, 0 unexpected failuresReview follow-up
NewStringFromUtf8Viewhelper in favor ofToV8ValueCopyV8StringToBuffertakes a pointer, matching project convention for mutating argumentsSigned-off-byare Yagiz Nizipli (human DCO)Assisted-by: Cursor