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

Make errors/error reporting more lightweight #1409

Merged
merged 3 commits into from Mar 12, 2021

Conversation

saethlin
Copy link
Contributor

Together, I measure these commits to be a 13% improvement in Message parsing speed. If there are changes in here that make anyone uncomfortable I'm happy to back them out. If they are indeed improvements, they'll become more obvious as I continue to knock time off the benchmark.

One other question I have is how much the quality of the error messages regresses once we no longer have the actual data in decode errors.

It looks like dropping the error payloads from DecodeError does not result in a measurable improvement, so I've put them back in.

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #1409 (b0cc115) into main (ebaba8b) will decrease coverage by 0.06%.
The diff coverage is 59.18%.

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   85.16%   85.10%   -0.06%     
==========================================
  Files         153      153              
  Lines       15035    15042       +7     
==========================================
- Hits        12804    12801       -3     
- Misses       2231     2241      +10     

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Overall I really like how simple these changes are. Based on your research, we probably want to make all Errors Box their Kinds?

Let's discuss the one defensive thing that was removed.

crates/proto/src/rr/domain/name.rs Show resolved Hide resolved
@saethlin
Copy link
Contributor Author

Overall I really like how simple these changes are. Based on your research, we probably want to make all Errors Box their Kinds?

I've been toying with ways to shrink the sizes of the error types, and yeah I think this is probably the best approach. There isn't really one type or pattern that's responsible for the error size situation; as far as I can tell it's mostly a bit of nesting here and a bit of nesting there. Nest just 2 levels and suddenly you've blown up the size at the top level. One could run around Boxing all the types that are responsible for the ErrorKind size, but doing that all over significantly alters the public interface, makes things harder to match on (no box patterns), and doesn't even shrink the size as much as possible by boxing the whole ErrorKind (without backtraces, usually 24 bytes by boxing members, 8 bytes with boxed ErrorKind).

@bluejekyll
Copy link
Member

Yup, I agree.

@djc, do you have any concnerns? I know you've been reviewing most of these changes, but this looks good to me.

@djc
Copy link
Collaborator

djc commented Mar 12, 2021

Most of this looks good to me. I'd probably have put the changes around read_character() in a separate commit, since they seem logically unrelated to the error types and it'd be good to record some rationale for these changes specifically in a commit message, but maybe not worth it now. Let's merge this!

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.

None yet

3 participants