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
perf: optimize error type #3596
base: master
Are you sure you want to change the base?
Conversation
I'm leaving this as a draft while I look into ways to maybe make |
H2 is probably the most likely to cause perf issues for various reasons, so I'm focusing on that case for now. |
Marked as ready and squashing to make review easier. Gonna leave as two commits and squash again on merge. |
We don't seem to need this boxed, and the allocs and frees do actually cost us a fair bit in prod according to my flamegraphs.
This seems to be costly as well for some applications, so let's avoid boxing unless we need to.
ef1672f
to
82df42d
Compare
Looks good to me. However, as it increases a little complexity, I would think that there would needs to be a benchmark to show in which cases it is actually efficient or not, and how the efficiency is. |
For context, the reason it was boxed before was:
|
I'll try and construct something, although I suspect that for most users this won't matter. That said, the effect of the double allocation is surprisingly significant. |
Hmm, the non-error case is a good point. I'll see if I can shrink this down. I'm investigating more right now the actual production issue I saw with this, because there's a chance that this might not be a valid use of HTTP/2 semantics I'm looking at which is causing this. I might potentially close this if I decide that any case where this matters is an issue of "you're holding it wrong". |
Did it turn out this isn't desired? |
The allocations and frees for these errors can get expensive in some cases. Any reduction in allocator usage here helps some use cases a lot it seems.