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

Speeding up the protocol parsing #2596

Merged
merged 3 commits into from
Mar 15, 2023
Merged

Speeding up the protocol parsing #2596

merged 3 commits into from
Mar 15, 2023

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Feb 23, 2023

This PR makes small changes to the protocol parsers. Specifically, this reduces the number of branches, and composite branches (if then if) that happen along the way.

As part of this change, fewer cast operations now happen, and the response comparison is (when necessary) only on the first byte of the response. This in turn makes the comparison cost lower (memory), and faster, due to the lack of repeated casting.

This is true of both async and sync.

There are no user facing changes.

@chayim chayim added the feature New feature label Feb 23, 2023
@chayim chayim requested a review from dvora-h February 23, 2023 14:58
redis/asyncio/connection.py Outdated Show resolved Hide resolved
]
elif byte not in (b"-", b"+", b":", b"$", b"*"):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this line could be just elif byte != b"+":. All other comparisons are known to be true here because none of the previous ifs/elifs matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ernest0x please take another look.. I've actually made it simpler.. and everything passes currently. But - I would love more eyes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chayim looks good. A minor thing could be that int(response) which is done in several places. Perhaps a variable could be set for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that doing that adds overhead and slows the parsing down (a bit, sure). This is one of those cases where the speed improvement dictates something that is less "PEP 1"... But then again, it follows the zen

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2023

Codecov Report

Base: 92.31% // Head: 92.26% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (a5416f4) compared to base (6c708c2).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2596      +/-   ##
==========================================
- Coverage   92.31%   92.26%   -0.06%     
==========================================
  Files         115      115              
  Lines       29744    29738       -6     
==========================================
- Hits        27459    27438      -21     
- Misses       2285     2300      +15     
Impacted Files Coverage Δ
redis/asyncio/connection.py 87.59% <100.00%> (-0.05%) ⬇️
redis/connection.py 86.74% <100.00%> (-0.05%) ⬇️
tests/conftest.py 84.94% <0.00%> (-0.78%) ⬇️
tests/test_asyncio/test_search.py 98.29% <0.00%> (-0.35%) ⬇️
redis/asyncio/cluster.py 91.68% <0.00%> (-0.33%) ⬇️
tests/test_cluster.py 96.64% <0.00%> (-0.33%) ⬇️
tests/test_asyncio/test_pubsub.py 99.37% <0.00%> (-0.16%) ⬇️
tests/test_asyncio/test_cluster.py 97.48% <0.00%> (-0.13%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chayim
Copy link
Contributor Author

chayim commented Feb 26, 2023

@dvora-h any idea why this hiredis test is failing? Have you seen this before? It shouldn't in any way be connected to these changes.

@dvora-h dvora-h merged commit 5588ae0 into master Mar 15, 2023
@dvora-h dvora-h deleted the ck-speeding-up-parsers branch March 15, 2023 09:35
aksinha334 pushed a commit to aksinha334/redis-py that referenced this pull request Mar 15, 2023
* speeding up the protocol parser

* linting

* changes to ease
dvora-h added a commit that referenced this pull request Mar 15, 2023
* Fixed issue #2598 - make Document class subscriptable

* Last time added older file, fixed it

* retrigger checks

* update json().arrindex() default values (#2611)

* update json().arrindex() default values

* add unit test

* fix falsy checks

* more unit tests

* add asyncio tests

* fix lint line length

---------

Co-authored-by: Alex Schmitz <aschmitz@box.com>

* Speeding up the protocol parsing (#2596)

* speeding up the protocol parser

* linting

* changes to ease

* Fixed CredentialsProvider examples (#2587)

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* ConnectionPool SSL example (#2605)

* [types] update return type of smismember to list[int] (#2617)

* update return type of smismember

* use Literal instead of int

* retrigger checks

* Added test for document subscriptable in tests/test_search.py

* Fixed linter issue

* retrigger checks

---------

Co-authored-by: Alex Schmitz <alex.schmitz@gmail.com>
Co-authored-by: Alex Schmitz <aschmitz@box.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Co-authored-by: Bar Shaul <88437685+barshaul@users.noreply.github.com>
Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>
Co-authored-by: CrimsonGlory <CrimsonGlory@users.noreply.github.com>
Co-authored-by: Raymond Yin <raymond@tryevergreen.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants