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.

Sign up for GitHub

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

Jump to bottom

Speeding up the protocol parsing #2596

Merged
merged 3 commits into from Mar 15, 2023
Merged

Speeding up the protocol parsing #2596

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fff888
speeding up the protocol parser
chayim Feb 23, 2023
5e716b2
linting
chayim Feb 23, 2023
a5416f4
changes to ease
chayim Feb 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 12 additions & 13 deletions redis/asyncio/connection.py
View file
Open in desktop
Expand Up @@ -267,9 +267,6 @@ async def _read_response(
response: Any
byte, response = raw[:1], raw[1:]

if byte not in (b"-", b"+", b":", b"$", b"*"):
raise InvalidResponse(f"Protocol Error: {raw!r}")

# server returned an error
if byte == b"-":
response = response.decode("utf-8", errors="replace")
Expand All @@ -291,19 +288,21 @@ async def _read_response(
elif byte == b":":
response = int(response)
# bulk response
elif byte == b"$":
length = int(response)
if length == -1:
return None
response = await self._read(length)
elif byte == b"$" and response == b"-1":
return None
elif byte == b"$" and response != b"-1":
response = await self._read(int(response))
# multi-bulk response
elif byte == b"*":
length = int(response)
if length == -1:
return None
elif byte == b"*" and response == b"-1":
return None
elif byte == b"*" and response != b"-1":
response = [
(await self._read_response(disable_decoding)) for _ in range(length)
(await self._read_response(disable_decoding))
for _ in range(int(response)) # noqa
]
elif byte not in (b"-", b"+", b":", b"$", b"*"):
chayim marked this conversation as resolved.
Show resolved Hide resolved
raise InvalidResponse(f"Protocol Error: {raw!r}")

if isinstance(response, bytes) and disable_decoding is False:
response = self.encoder.decode(response)
return response
Expand Down
24 changes: 11 additions & 13 deletions redis/connection.py
View file
Open in desktop
Expand Up @@ -358,9 +358,6 @@ def _read_response(self, disable_decoding=False):

byte, response = raw[:1], raw[1:]

if byte not in (b"-", b"+", b":", b"$", b"*"):
raise InvalidResponse(f"Protocol Error: {raw!r}")

# server returned an error
if byte == b"-":
response = response.decode("utf-8", errors="replace")
Expand All @@ -381,20 +378,21 @@ def _read_response(self, disable_decoding=False):
elif byte == b":":
response = int(response)
# bulk response
elif byte == b"$":
length = int(response)
if length == -1:
return None
response = self._buffer.read(length)
elif byte == b"$" and response == b"-1":
return None
elif byte == b"$" and response != b"-1":
response = self._buffer.read(int(response))
# multi-bulk response
elif byte == b"*":
length = int(response)
if length == -1:
return None
elif byte == b"*" and response == b"-1":
return None
elif byte == b"*" and response != b"-1":
response = [
self._read_response(disable_decoding=disable_decoding)
for i in range(length)
for i in range(int(response))
]
elif byte not in (b"-", b"+", b":", b"$", b"*"):
Copy link

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.

Copy link

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

raise InvalidResponse(f"Protocol Error: {raw!r}")

if isinstance(response, bytes) and disable_decoding is False:
response = self.encoder.decode(response)
return response
Expand Down