search

hyperium / hyper

An HTTP library for Rust
https://hyper.rs
MIT License
14.42k stars 1.59k forks source link

perf: optimize error type #3596

Closed Noah-Kennedy closed 4 months ago

Noah-Kennedy commented 6 months ago

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.

Noah-Kennedy commented 6 months ago

I'm leaving this as a draft while I look into ways to maybe make Cause faster as well.

Noah-Kennedy commented 6 months ago

H2 is probably the most likely to cause perf issues for various reasons, so I'm focusing on that case for now.

Noah-Kennedy commented 6 months ago

Marked as ready and squashing to make review easier. Gonna leave as two commits and squash again on merge.

tottoto commented 6 months ago

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.

seanmonstar commented 6 months ago

For context, the reason it was boxed before was:

Noah-Kennedy commented 6 months ago

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.

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.

Noah-Kennedy commented 6 months ago

For context, the reason it was boxed before was:

* To reduce return sizes of `Result<T, Error>`.

* Because errors should be less common, it seemed better to make the error case more expensive if it allowed the good case to be any faster.

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".

seanmonstar commented 5 months ago

Did it turn out this isn't desired?