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
Optimize name parsing #1388
Optimize name parsing #1388
Conversation
Codecov Report
@@ 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 |
9ee9091
to
bd4bf60
Compare
At this point, I'm stuck. This implementation grows the size of However, this size increase has caused I also cannot change the representation of @bluejekyll Do you have any suggestions? |
Ditching the |
6a53d0b
to
4d29b04
Compare
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. |
I agree, but if we're looking to decrease the memory footprint of |
I'm personally fine with prioritizing security over memory usage. |
As a nice side effect, this is also ~5% faster in a microbenchmark. |
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? |
The previous implementation used Rc to represent Label, then composed those in an array to represent Name. That produced a large number of small allocations in the parsing code path. This new implementation avoids allocations entirely for small names, and unless the name has a very large number of labels, it is stored entirely in one allocation. This also removes the Index impl for Name. Since we no longer contain any Labels, we cannot implement that (a common problem with Index leaking implementation details).
b5026d1
to
7b6b79a
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Commented on some minor nits, but I definitely think we should merge something like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forgot to submit this earlier.
@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?) |
I'm not convinced this is totally spurious? It's failed in the same way the last two commits in this PR 🤷 |
I kicked the CI jobs again, we'll see what happens. |
FYI, I reran the tests twice, I think we're good. Merging in. Thanks for this PR and all the perf improvements! |
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 makingWe're going with the flatvec-style approach to make better use of the expanded stack footprint.Name
larger) because it avoid allocations entirely for a lot of common domain names; 4 labels or less all 16 bytes or less.Before:
After: