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

Fix performance regression when parsing bytes. #1909

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

rsmmr
Copy link
Member

@rsmmr rsmmr commented Oct 25, 2024

Turns out our improved error messages were adding additional overhead
because we were now constructing them through fmt() each time we
needed more data, independent of whether there was actually going to
be an error reported.

This adds a second version of waitForInput() that doesn't receive an
already prepared error message, but just returns false on error so
that the caller can throw itself.

Closes #1908.

Turns out our improved error messages were adding additional overhead
because we were now constructing them through `fmt()` each time we
needed more data, independent of whether there was actually going to
be an error reported.

This adds a second version of `waitForInput()` that doesn't receive an
already prepared error message, but just returns false on error so
that the caller can throw itself.

Closes #1908.
Copy link
Contributor

@evantypanski evantypanski left a comment

Choose a reason for hiding this comment

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

Would it make sense to expand this more in the future so it doesn't come up again? There's a doc comment now, which is good, but more guarantees to prevent us from shooting ourselves in the foot could be nice in the future (not here though, probably)

Something like a lazy-evaluated fmt would be really cool. But probably not necessary :)

@rsmmr
Copy link
Member Author

rsmmr commented Oct 28, 2024

Would it make sense to expand this more in the future so it doesn't come up again?

Yeah, good point. In fact I was trying to come up with something that would enforce correct usage, but couldn't think of anything viable that wouldn't beed larger changes. My two ideas were: (1) leveraging the type system by passing a const char* so that the result of fmt() wouldn't be accepted, and (2) passing a lambda instead of the error message, similar to your idea of a lazy-evaluated fmt. But (1) broke other stuff with generated code passing string_views; and (2) I think would itself be too expensive because it would now need to capture each time. Let me know if you have other ideas!

@rsmmr rsmmr merged commit e179079 into main Oct 28, 2024
20 checks passed
@rsmmr rsmmr deleted the topic/robin/gh-1908-perf-regression branch October 28, 2024 09:54
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.

Optimize parsing for bytes &size=N regressed QUIC performance
3 participants