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

optimizing is_ipv4 #561

Merged
merged 5 commits into from
Nov 13, 2023
Merged

optimizing is_ipv4 #561

merged 5 commits into from
Nov 13, 2023

Conversation

lemire
Copy link
Member

@lemire lemire commented Nov 13, 2023

This PR optimizes the is_ipv4 function by first checking the last character. Unless the last character is a hex number or the letter x, we can just return false immediately and save quite a bit of work.

The function was also rewritten with more safeguards.

LLVM 14, Apple M2.

Before:

BasicBench_AdaURL_href              24053490 ns     24053414 ns           29 GHz=3.38364 cycle/byte=9.37729 cycles/url=814.504 instructions/byte=38.9054 instructions/cycle=4.14889 instructions/ns=14.0383 instructions/url=3.37929k ns/url=240.719 speed=361.2M/s time/byte=2.76855ns time/url=240.474ns url/s=4.15845M/s
BasicBench_AdaURL_aggregator_href   16111354 ns     16111372 ns           43 GHz=3.48936 cycle/byte=6.19904 cycles/url=538.444 instructions/byte=27.773 instructions/cycle=4.48021 instructions/ns=15.6331 instructions/url=2.41234k ns/url=154.31 speed=539.252M/s time/byte=1.85442ns time/url=161.073ns url/s=6.20835M/s

After

BasicBench_AdaURL_href              24135053 ns     24135033 ns           30 GHz=3.39617 cycle/byte=9.33507 cycles/url=810.837 instructions/byte=38.7939 instructions/cycle=4.15571 instructions/ns=14.1135 instructions/url=3.3696k ns/url=238.751 speed=359.978M/s time/byte=2.77794ns time/url=241.29ns url/s=4.14439M/s
BasicBench_AdaURL_aggregator_href   15683330 ns     15683273 ns           44 GHz=3.3813 cycle/byte=6.09465 cycles/url=529.377 instructions/byte=27.3717 instructions/cycle=4.49111 instructions/ns=15.1858 instructions/url=2.37749k ns/url=156.56 speed=553.972M/s time/byte=1.80515ns time/url=156.794ns url/s=6.37781M/s

@lemire lemire requested a review from anonrig November 13, 2023 18:20
@anonrig
Copy link
Member

anonrig commented Nov 13, 2023

I think we have a bug in here. This is somewhat similar to nodejs/node#50632

D:\a\ada\ada\tests\wpt_tests.cpp(385): error: Expected equality of these values:
  input_url->get_host()
    Which is: "256"
  host
    Which is: "0.0.1.0"

@lemire
Copy link
Member Author

lemire commented Nov 13, 2023

@anonrig Seems unrelated to this Node issue unless I misunderstand something. I have a Windows box open in front of me and I am failing to reproduce this issue.

@lemire
Copy link
Member Author

lemire commented Nov 13, 2023

The failing test is this...

{
    "input": "http://256",
    "base": "http://other.com/",
    "href": "http://0.0.1.0/",
    "origin": "http://0.0.1.0",
    "protocol": "http:",
    "username": "",
    "password": "",
    "host": "0.0.1.0",
    "hostname": "0.0.1.0",
    "port": "",
    "pathname": "/",
    "search": "",
    "hash": ""
  }

But this test is not new. And I cannot reproduce it locally.

And it is only triggered by Visual Studio in release mode. And only in the last few commits.

Weird.

What could it be?

@lemire lemire changed the title changing Visual Studio test matrix in CI optimizing is_ipv4 Nov 13, 2023
@lemire
Copy link
Member Author

lemire commented Nov 13, 2023

@anonrig I am merging this. We can revert later if you disagree.

@lemire lemire merged commit 309c285 into main Nov 13, 2023
35 checks passed
@lemire lemire deleted the changing_vs_ci_matrix branch November 13, 2023 22:17
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