Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: improve ends_in_a_number by 25% #36

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jan 6, 2023

Before

BasicBench_AdaURL           10253 ns        10161 ns        69455 time/byte=13.3522ns

After

BasicBench_AdaURL            7374 ns         7341 ns        94908 time/byte=9.64605ns

@@ -7,34 +7,55 @@ namespace ada::checkers {

// TODO: Refactor this to not use `std::vector` but use pointer arithmetic for performance.
bool ends_in_a_number(const std::string_view input) noexcept {
// Let parts be the result of strictly splitting input on U+002E (.).
std::vector<std::string> parts = ada::helpers::split_string_view(input, '.', false);
Copy link
Member

Choose a reason for hiding this comment

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

Getting rid of std::vector in this code is assuredly a good step forward. Please merge.

std::vector will tend to allocate on the heap. You want to avoid that as much as possible. It is expensive in our context.

@anonrig anonrig merged commit 8d1ddc5 into main Jan 6, 2023
@anonrig anonrig deleted the improve-ends-in-a-number branch January 6, 2023 23:50
return true;
}

// If parsing last as an IPv4 number does not return failure, then return true.
return ada::parser::parse_ipv4_number(last).has_value();
return is_ipv4_number_valid(std::string(pointer_start, pointer_end));
Copy link
Member Author

Choose a reason for hiding this comment

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

Next step will be to remove this. I'm working on it.

miguelteixeiraa added a commit to miguelteixeiraa/node that referenced this pull request Jan 16, 2023
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Jan 23, 2023
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit to nodejs/node that referenced this pull request Feb 1, 2023
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this pull request Mar 3, 2023
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this pull request Mar 5, 2023
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.

2 participants