Skip to content

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Dec 9, 2025

This would significantly improve URLPattern performance

@anonrig anonrig requested review from CarlosEduR and lemire December 9, 2025 15:11
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 9, 2025

CodSpeed Performance Report

Merging #1033 will degrade performances by 99.99%

Comparing yagiz/optimize-canonicalize-methods (5da3603) with main (236e519)

Summary

❌ 1 regression
✅ 18 untouched
⏩ 3 skipped1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
BasicBench_whatwg 31.7 µs 213,680.4 µs -99.99%

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@anonrig anonrig force-pushed the yagiz/optimize-canonicalize-methods branch from 792e6ed to 5da3603 Compare December 10, 2025 03:08
@anonrig
Copy link
Member Author

anonrig commented Dec 10, 2025

@lemire can you review?

Copy link
Member

@CarlosEduR CarlosEduR left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'd still like Daniel to take a look.

if (!url->set_username(input)) {
return tl::unexpected(errors::type_error);
// Percent-encode the input using the userinfo percent-encode set.
size_t idx = ada::unicode::percent_encode_index(
Copy link
Member

Choose a reason for hiding this comment

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

good.

@anonrig anonrig merged commit efbd82e into main Dec 11, 2025
50 of 52 checks passed
@anonrig anonrig deleted the yagiz/optimize-canonicalize-methods branch December 11, 2025 20:08

if (result.ec == std::errc()) {
// Successfully parsed, return as string
return std::to_string(parsed_port);
Copy link
Member

Choose a reason for hiding this comment

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

Presumably, this could be avoided by using the range digits_to_parse.data() to digits_to_parse.data() + digits_to_parse.size() as it should be a valid number string. Although it might be not necessary.

The whole logic here is to ensure that we have a 16-bit unsigned string. This could be better.

Still: this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nice touch. Agreed. Let's fix it! I'll open a PR

Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants