Optimize network address parser

[niklasf]

Motivated by working with datasets of IP addresses.

First commit adds more examples for each case in the benchmark suite.

Second commit splits read_number() into seperate methods for the the max_digits Some(_) and None cases, which by itself significantly helps code generation.

Hoist reading the first digit which is always required.

The path for unlimited digits then no longer has to count digits at all.

Drive-by "fixes" the 10 digit debug assertion (which would be insufficient in base 16). Not actually used anywhere near the limit.

./x bench library/coretests --test-args addr_parser before -> after on AMD Ryzen 9 9950X:

net::addr_parser::bench_parse_ipaddr_v4           81.73ns/iter  +/- 0.60  ->   62.36ns/iter +/- 0.12
net::addr_parser::bench_parse_ipaddr_v6_compress 303.70ns/iter  +/- 1.29  ->  212.80ns/iter +/- 1.25
net::addr_parser::bench_parse_ipaddr_v6_full     453.06ns/iter  +/- 7.89  ->  294.88ns/iter +/- 0.91
net::addr_parser::bench_parse_ipaddr_v6_v4       301.41ns/iter  +/- 1.52  ->  224.82ns/iter +/- 0.99
net::addr_parser::bench_parse_ipv4                87.24ns/iter  +/- 0.32  ->   52.94ns/iter +/- 0.33
net::addr_parser::bench_parse_ipv6_compress      277.92ns/iter  +/- 3.29  ->  195.98ns/iter +/- 1.10
net::addr_parser::bench_parse_ipv6_full          402.48ns/iter +/- 22.20  ->  276.37ns/iter +/- 4.05
net::addr_parser::bench_parse_ipv6_v4            276.78ns/iter  +/- 1.18  ->  216.04ns/iter +/- 1.20
net::addr_parser::bench_parse_socket_v4          105.18ns/iter  +/- 0.79  ->   69.20ns/iter +/- 0.46
net::addr_parser::bench_parse_socket_v6          378.69ns/iter  +/- 5.25  ->  274.82ns/iter +/- 0.95
net::addr_parser::bench_parse_socket_v6_scope_id 336.61ns/iter  +/- 2.36  ->  258.56ns/iter +/- 0.89
net::addr_parser::bench_parse_socketaddr_v4      118.20ns/iter  +/- 0.66  ->   78.86ns/iter +/- 0.22
net::addr_parser::bench_parse_socketaddr_v6      429.17ns/iter  +/- 5.70  ->  315.69ns/iter +/- 1.48

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 11 candidates
• Random selection from 6 candidates

[workingjubilee]

Uh, if joboet has an opinion otherwise, that's cool, but if we're going to hoist out the decimal implementation into its own thing, can't we just replace read_number with read_decimal and read_hex instead of this read_radix_max_digits function? It's only ever called with those two radixes and is internal anyways?

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  IMAGE: x86_64-gnu-tools
##[endgroup]
    Updating crates.io index
error: failed to get `simd-adler32` as a dependency of package `miniz_oxide v0.8.8`
    ... which satisfies dependency `miniz_oxide = "^0.8.5"` of package `flate2 v1.1.9`
    ... which satisfies dependency `flate2 = "^1.1.9"` of package `citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)`

Caused by:
  failed to load source for dependency `simd-adler32`

Caused by:

[niklasf]

replace read_number with read_decimal and read_hex

Tried that initially, but splitting across max_digits vs. unlimited appears to be more impactful than splitting across radix.

Other feedback applied and ready for the next round of review.

@rustbot ready

[workingjubilee]

can you quantify "more impactful"?

[workingjubilee]

Or like, broadly describe even? "more impactful" doesn't really tell me anything.

[workingjubilee]

And my apologies about the initial inquiry, I only felt compelled to ask because I truly felt baffled by the sentence. I was at various levels of "surely that's not what you meant?" with myself and "I have to ask, but...?" and the simplification/explained version is quite understandable.

[niklasf]

No worries, and thanks for still leaving a review in the first place.

read_hex() ends up with exactly one call-site in IPv6 parsing, while all the other features are still required for read_decimal(). Basically:

fn read_decimal<T: ReadNumberHelper + TryFrom<u32>>(&mut self, max_digits: Opton<usize>, allow_zero_prefix: bool) -> Option<T> { ... }
fn read_4_hex_digits(&mut self) -> Option<u16> { ... }

With that I measure:

net::addr_parser::bench_parse_ipaddr_v4           73.16ns/iter  +/- 1.39
net::addr_parser::bench_parse_ipaddr_v6_compress 217.49ns/iter  +/- 1.83
net::addr_parser::bench_parse_ipaddr_v6_full     336.63ns/iter +/- 10.53
net::addr_parser::bench_parse_ipaddr_v6_v4       255.87ns/iter  +/- 1.55
net::addr_parser::bench_parse_ipv4                71.75ns/iter  +/- 0.35
net::addr_parser::bench_parse_ipv6_compress      200.89ns/iter  +/- 0.78
net::addr_parser::bench_parse_ipv6_full          299.51ns/iter  +/- 1.16
net::addr_parser::bench_parse_ipv6_v4            229.38ns/iter  +/- 3.10
net::addr_parser::bench_parse_socket_v4           91.30ns/iter  +/- 0.30
net::addr_parser::bench_parse_socket_v6          282.56ns/iter  +/- 0.94
net::addr_parser::bench_parse_socket_v6_scope_id 264.66ns/iter  +/- 1.69
net::addr_parser::bench_parse_socketaddr_v4       93.73ns/iter  +/- 0.20
net::addr_parser::bench_parse_socketaddr_v6      326.19ns/iter  +/- 1.47

[workingjubilee]

There may be a more "proper" way to rerun the PR CI but every time I am told about it I recall thinking, "That seems like more effort, though?"

read_hex() ends up with exactly one call-site in IPv6 parsing,

Right, I had an optimistic assumption that code duplication might actually be fine here for performance reasons.

It looks like the numbers are... rereads the tables a couple times. Oh. I see! That is indeed less impactful. The benchmarking framework itself is not exactly the best implementation of benchmarking, so I do wonder slightly about variance, but it's good enough that this is a clear improvement. Thanks!

...I wonder if it's because it's actually routing less logic through sufficiently-similar-looking codepaths so that LLVM infers the optimization is less important...? Or it's just not as easy for CSE/GVN...? Well, either way.

r? me
@bors r+

View changes since this review

[rust-bors]

📌 Commit 1d54265 has been approved by workingjubilee

It is now in the queue for this repository.