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: Remove unnecessary string casting of bytes #276

Merged
merged 4 commits into from
Nov 16, 2024

Conversation

BYK
Copy link
Contributor

@BYK BYK commented Nov 14, 2024

@Kriechi
Copy link
Member

Kriechi commented Nov 15, 2024

To call out the explicit: is this a behaviour change or a performance improvement?

If its a performance related change, I would like to see a benchmark showing "time spent" or similar metrics that changed.

@BYK
Copy link
Contributor Author

BYK commented Nov 15, 2024

Great call! Thanks to your prompt I actually timed the new version and improved it even further (code will be pushed when you are reading this).

So this should keep the same behavior in terms of types. The caveat is, the original version always returns a new bytes object due to the bytes -> str -> bytes conversion whereas the new version simply returns the same object if it is already bytes. This should be okay, even good as the intent was never to get a new copy but worth calling out. Otherwise this is purely a performance optimization with the following numbers where _to_bytes is the old version and _to_bytes_new is the new version proposed in this PR:

>>> s
'someheader'
>>> b
b'someheader'
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.22222996300115483
0.16239697000128217
0.18601619399851188
0.1108450250030728
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.20466226000280585
0.16354602499632165
0.1937305280007422
0.12870584500342375
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.20280992899643024
0.16408630299702054
0.1972411400056444
0.12873706800019136
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.21261542499996722
0.16578178100462537
0.18726225899445126
0.15498205900075845

As you can see it is consistently faster than the old version for both str and bytes inputs and significantly faster for bytes inputs which is what we should be dealing with after python-hyper/h2#1286

These numbers are using Python 3.10.12. When using Python 3.12.4, they are even better:

>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.13931607300037285
0.1050484180013882
0.11498607300018193
0.07148081599734724
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.14760045900038676
0.0974436950054951
0.11569900900212815
0.06948418200045126
>>> timeit(lambda: _to_bytes(s));timeit(lambda: _to_bytes(b));timeit(lambda: _to_bytes_new(s));timeit(lambda: _to_bytes_new(b))
0.13979562999884365
0.09725762299785856
0.11685396800021408
0.075411345998873

src/hpack/hpack.py Outdated Show resolved Hide resolved
@Kriechi
Copy link
Member

Kriechi commented Nov 16, 2024

@BYK I would also be happy to hear your thoughts about #277 which I just sent - and if it changes anything or points out an previously hidden issue!

@BYK BYK requested a review from Kriechi November 16, 2024 16:52
@BYK
Copy link
Contributor Author

BYK commented Nov 16, 2024

@BYK I would also be happy to hear your thoughts about #277 which I just sent - and if it changes anything or points out an previously hidden issue!

Oooh nice! I can rebase my patch on top of that if you wanna go ahead and merge it before. I'll give that a proper look this weekend.

@Kriechi Kriechi merged commit e574f0d into python-hyper:master Nov 16, 2024
0 of 6 checks passed
@BYK BYK deleted the byk/fix/byte-str-cast branch November 16, 2024 19:16
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.

2 participants