encode / httpcore

A minimal HTTP client. ⚙️
https://www.encode.io/httpcore/
BSD 3-Clause "New" or "Revised" License
465 stars 105 forks source link

Remove expensive run time type casting #856

Closed bdraco closed 11 months ago

bdraco commented 11 months ago

Summary

Guard casting type subscripting with TYPE_CHECKING to avoid runtime construction each time _receive_event is called which is unexpectedly expensive

import h11
from typing import TYPE_CHECKING, Union, Type, cast
import timeit

def test_with_type_checking():
    # mypy fails to narrow the type in the above if statement above
    event = None
    if TYPE_CHECKING:  # pragma: no cover
        event = cast(Union[h11.Event, Type[h11.PAUSED]], event)  # pragma: no cover

def test_without_type_checking():
    event = None
    event = Union[h11.Event, Type[h11.PAUSED]], event

without_type_checking = timeit.timeit(test_without_type_checking)
with_type_checking = timeit.timeit(test_with_type_checking)

print(f"Time for without: {without_type_checking}")
print(f"Time for with: {with_type_checking}")
Time for without: 0.37302987500152085
Time for with: 0.02093412500107661
Screenshot 2023-12-04 at 11 15 24 AM Screenshot 2023-12-04 at 11 15 14 AM

Checklist

T-256 commented 11 months ago

Seems reasonable! how can I reproduce?

bdraco commented 11 months ago

I only saw it because it came up in the py-spy watching Home Assistant request for an enphase envoy. It shows up in a callgrind as well using cProfile.

bdraco commented 11 months ago

Added a micro benchmark above

bdraco commented 11 months ago

Yep, smart change. 👍 For review I grep'ed this and ensured that you've applied this to all the cast cases consistently.

An alternative here would be to remove the cast and use a type: ignore, should we consider that instead as a simpler alternative? (Or not, also okay?)

Happy to do either as its really a matter of preference.

I'm used to guarding with if TYPE_CHECKING since thats what Home Assistant prefers for these cases, but some projects prefer ignores.

T-256 commented 11 months ago

but some projects prefer ignores.

I do think this repo also prefers ignores over casting. I'm unsure why cast added to this line but it's worth to use type: ignore instead at there.

bdraco commented 11 months ago

Thanks