Skip to content

Refactor fallback' read header #441

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

Merged
merged 6 commits into from
Jan 2, 2021

Conversation

GuyTuval
Copy link
Contributor

@GuyTuval GuyTuval commented Oct 22, 2020

I refactored the code in the _read_header() method of the fallback.py file.
I removed duplicated code and (hopefully) made the function more readable and maintainable.

@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #441 (b02d115) into master (8fb709f) will decrease coverage by 0.26%.
The diff coverage is 97.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   88.07%   87.80%   -0.27%     
==========================================
  Files           5        5              
  Lines         847      730     -117     
==========================================
- Hits          746      641     -105     
+ Misses        101       89      -12     
Impacted Files Coverage Δ
msgpack/fallback.py 88.81% <97.87%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fb709f...b02d115. Read the comment docs.

@GuyTuval GuyTuval force-pushed the refactor-fallback'-read-header branch 2 times, most recently from d8c9f91 to 21d6a1c Compare October 27, 2020 23:52
@GuyTuval
Copy link
Contributor Author

Hey @methane!
I saw that you are one of the contributors to this repo.
I wanted to ask if you would mind review this PR? 😄

@methane
Copy link
Member

methane commented Oct 30, 2020

Would you try benchmark/benchmark.py ?
Run the script with master branch and your PR branch.

@GuyTuval GuyTuval force-pushed the refactor-fallback'-read-header branch from 21d6a1c to 259f343 Compare October 30, 2020 10:23
@GuyTuval
Copy link
Contributor Author

Would you try benchmark/benchmark.py ?
Run the script with master branch and your PR branch.
I ran the tests on my system.
The results with master branch:

packing integers (ext)           0.340488, 0.340425, 0.343065, 0.348732
packing integers (fallback)      4.452573, 4.616886, 4.559352, 4.515878
unpacking integers (ext)         0.066253, 0.066310, 0.068629, 0.069208
unpacking integers (fallback)    8.194786, 8.229670, 8.296369, 8.541644
packing bytes (ext)              0.034188, 0.034607, 0.034281, 0.034365
packing bytes (fallback)         0.737407, 0.747305, 0.775749, 0.839676
unpacking bytes (ext)            0.024286, 0.024064, 0.023509, 0.023577
unpacking bytes (fallback)       2.422867, 2.279277, 2.227503, 2.189443
packing lists (ext)              0.738786, 0.733464, 0.731821, 0.717495
packing lists (fallback)       10.754110, 10.833690, 11.016210, 10.988131
unpacking lists (ext)            0.287086, 0.288698, 0.291442, 0.286039
unpacking lists (fallback)     12.463816, 12.133081, 11.914829, 11.711606
packing dicts (ext)              0.438501, 0.456876, 0.459528, 0.449412
packing dicts (fallback)       12.814651, 12.784222, 12.757698, 12.723056
unpacking dicts (ext)            0.258662, 0.269982, 0.260755, 0.263075
unpacking dicts (fallback)     11.955657, 12.018308, 11.945628, 11.894115

The result with my PR barnch:

packing integers (ext)           0.333752, 0.339650, 0.347493, 0.359014
packing integers (fallback)      4.536829, 4.540393, 4.569847, 4.691006
unpacking integers (ext)         0.066953, 0.067022, 0.066793, 0.067799
unpacking integers (fallback)    8.337325, 8.405046, 8.332201, 8.512055
packing bytes (ext)              0.034619, 0.035266, 0.036648, 0.036447
packing bytes (fallback)         0.762590, 0.793024, 0.764875, 0.745429
unpacking bytes (ext)            0.023080, 0.022725, 0.022505, 0.022861
unpacking bytes (fallback)       2.303375, 2.322663, 2.262317, 2.418983
packing lists (ext)              0.711621, 0.721075, 0.726628, 0.727309
packing lists (fallback)       10.523426, 10.661116, 10.569996, 10.661933
unpacking lists (ext)            0.301531, 0.293438, 0.296860, 0.307903
unpacking lists (fallback)     11.514852, 11.516598, 11.434329, 11.441009
packing dicts (ext)              0.416874, 0.415810, 0.425535, 0.435006
packing dicts (fallback)       12.687544, 12.636342, 12.754518, 12.723379
unpacking dicts (ext)            0.260288, 0.256923, 0.256135, 0.258123
unpacking dicts (fallback)     11.887248, 11.940982, 11.871376, 11.797869

@GuyTuval GuyTuval force-pushed the refactor-fallback'-read-header branch from 259f343 to 5eb8c5f Compare October 30, 2020 10:42
@methane
Copy link
Member

methane commented Oct 30, 2020

OK, would you try the benchmark with PyPy too?
As far as I remember, PyPy does spetial optimization for struct.unpack(). I am afraid that moving format from literal to variable makes the optimization doesn't work.

Note that, extension can be used by CPython when performance is needed, but fallback is used by PyPy always.

@GuyTuval GuyTuval closed this Oct 30, 2020
@GuyTuval GuyTuval reopened this Oct 30, 2020
@GuyTuval
Copy link
Contributor Author

OK, would you try the benchmark with PyPy too?
As far as I remember, PyPy does spetial optimization for struct.unpack(). I am afraid that moving format from literal to variable makes the optimization doesn't work.

Note that, extension can be used by CPython when performance is needed, but fallback is used by PyPy always.

I tried with PyPy 2.7.
With master branch:

packing integers (fallback)      0.186303, 0.176621, 0.170210, 0.169829
unpacking integers (fallback)    0.078988, 0.058202, 0.057949, 0.058773
packing bytes (fallback)         0.078529, 0.041171, 0.039028, 0.041117
unpacking bytes (fallback)       0.136437, 0.094988, 0.095692, 0.097415
packing lists (fallback)         0.231822, 0.204713, 0.201140, 0.205309
unpacking lists (fallback)       0.512283, 0.486721, 0.487005, 0.491213
packing dicts (fallback)         0.320112, 0.277198, 0.271035, 0.268374
unpacking dicts (fallback)       0.386129, 0.369852, 0.376348, 0.370727

with my PR branch:

packing integers (fallback)      0.192539, 0.175923, 0.163118, 0.160464
unpacking integers (fallback)    0.078167, 0.057289, 0.058188, 0.059882
packing bytes (fallback)         0.073200, 0.040191, 0.041237, 0.041831
unpacking bytes (fallback)       0.158312, 0.110172, 0.103099, 0.102477
packing lists (fallback)         0.221874, 0.201449, 0.195479, 0.199038
unpacking lists (fallback)       0.527486, 0.506253, 0.490690, 0.529199
packing dicts (fallback)         0.295316, 0.262627, 0.258951, 0.256411
unpacking dicts (fallback)       0.405497, 0.399488, 0.382478, 0.382264

@GuyTuval
Copy link
Contributor Author

GuyTuval commented Nov 2, 2020

Hi @methane! Any other checks are needed?
If there aren't would you mind merge my PR?

@methane
Copy link
Member

methane commented Nov 11, 2020

I don't have time to look it in this year. Please wait months.

@GuyTuval
Copy link
Contributor Author

Hello @methane 😀
Can you check out this PR?

@GuyTuval GuyTuval force-pushed the refactor-fallback'-read-header branch from bdfbee6 to 3fc7075 Compare January 1, 2021 10:55
@GuyTuval GuyTuval force-pushed the refactor-fallback'-read-header branch from 3fc7075 to b02d115 Compare January 1, 2021 10:59
@GuyTuval GuyTuval requested a review from methane January 1, 2021 11:09
@methane methane merged commit 3b71818 into msgpack:master Jan 2, 2021
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.

4 participants