Optimize name parsing

[saethlin]

This is deliberately a draft. Looking to investigate an approach like flatvec too, but my instinct is that this approach is better (at the expense of making Name larger) because it avoid allocations entirely for a lot of common domain names; 4 labels or less all 16 bytes or less. We're going with the flatvec-style approach to make better use of the expanded stack footprint.

Before:

test bench_parse_real_message          ... bench:       4,219 ns/iter (+/- 38)

After:

test bench_parse_real_message          ... bench:       1,780 ns/iter (+/- 16)

[codecov]

Codecov Report

Merging #1388 (c6ac502) into main (a6eb537) will decrease coverage by 0.01%.
The diff coverage is 96.47%.

@@            Coverage Diff             @@
##             main    #1388      +/-   ##
==========================================
- Coverage   85.21%   85.20%   -0.01%     
==========================================
  Files         153      152       -1     
  Lines       15016    15020       +4     
==========================================
+ Hits        12795    12797       +2     
- Misses       2221     2223       +2     

[saethlin]

At this point, I'm stuck.

This implementation grows the size of Name significantly, because it adds stack buffers to avoid heap allocations. Avoiding allocations is the primary cause of improved performance here.

However, this size increase has caused ResolveErrorKind to grow significantly (the size was already too large, this PR makes it much worse). We could paper over the impact on the size of ResolveError by giving it the same #[cold] + Box treatment, but that doesn't address the lint, which is firing on an enum whose representation is public, so boxing its components would be a breaking change.

I also cannot change the representation of Name to use the flatvec approach, which would significantly improve the stack footprint. Name needs to contain a collection of Label somewhere because of its Index impl (Name::iter is fine because it yields slices of bytes). If I had my way I'd toss the Index impls.

@bluejekyll Do you have any suggestions?

[djc]

Ditching the Index impls sounds good to me provided we don't rely on them too much in tree?

[djc]

BTW, I know smallvec is currently used but maybe we should take the chance to migrate to tinyvec. smallvec has a pretty bad security record, with 5 RustSec-reported vulnerabilities reported over the past three years.

[saethlin]

I agree, but if we're looking to decrease the memory footprint of Name, going to tinyvec takes us in the wrong direction. If we're not worried overmuch about that, I'm all in. Perhaps I'm just reacting to a lint that we should add an allow for in this PR then fix soon after when we actually address the situation with errors. TinyVec can accept an inline buffer up to 24 bytes without growing, but itself has a minimum size of 32, where SmallVec has a minimum size of 24, the same as Vec.

[djc]

I'm personally fine with prioritizing security over memory usage.

[saethlin]

As a nice side effect, this is also ~5% faster in a microbenchmark.

[bluejekyll]

Where are we on this? I reviewed. I like the changes better than the one I had been working on. Is there still a concern on additional stack space?

[saethlin]

@bluejekyll I think this is good to go. I've rebased this into two commits to erase the previous implementation strategy and preserve the benchmark on its own so people can check it out and run against the old code. I also bumped one of the inline buffers up to 24 bytes because it's free.

I do not think there are any outstanding concerns about stack footprint. I think the current implementation is a good compromise, and the consequence it has on the size of the error types that was tripping lints is a bigger problem that I'm tackling in another PR.

[djc]

This is looking great! Commented on some minor nits, but I definitely think we should merge something like this.

[djc]

Sorry, forgot to submit this earlier.

[djc]

@bluejekyll I think this can just be merged, but it doesn't look like I have privileges to do so because CI failed (from what I can tell, spuriously). (Maybe remove the requirement that CI passes necessarily?)

[saethlin]

CI failed (from what I can tell, spuriously). (Maybe remove the requirement that CI passes necessarily?)

I'm not convinced this is totally spurious? It's failed in the same way the last two commits in this PR 🤷

[bluejekyll]

I kicked the CI jobs again, we'll see what happens.

[bluejekyll]

FYI, I reran the tests twice, I think we're good. Merging in. Thanks for this PR and all the perf improvements!