chore: Introduce small buffer in redis parser #4076
Conversation
src/facade/redis_parser.cc
Outdated
| InitStart(str[0], res); | ||
| } | ||
| DCHECK(state_ != CMD_COMPLETE_S); |
There was a problem hiding this comment.
I had to check this. InitStart does update state_ if it's equal CMD_COMPLETE_S so it's fine.
src/facade/redis_parser.cc
Outdated
| if (pos[-1] != '\r') { | ||
| return BAD_INT; | ||
|
|
||
| consumed = pos - s + 1; |
There was a problem hiding this comment.
I am blindly trusting you here. I haven;t checked the index calculations but the tests are failing and it's a sign that there is a mistake somewhere around here 😄
|
Tests are failing :) |
romange
force-pushed
the
Parser
branch
3 times, most recently
from
759f1e7 to
93682c7
Compare
romange
force-pushed
the
Parser
branch
9 times, most recently
from
dbf8419 to
6ce0516
Compare
romange
force-pushed
the
Parser
branch
4 times, most recently
from
e84c303 to
b788c86
Compare
romange
force-pushed
the
Parser
branch
3 times, most recently
from
63a3bf1 to
f7fa90c
Compare
|
@romange still not green |
|
And I still have not requested to review :) |
Uhhh I saw the notification and saw you asked 😄 nvm |
romange
force-pushed
the
Parser
branch
7 times, most recently
from
002d26d to
992fbe8
Compare
romange
force-pushed
the
Parser
branch
4 times, most recently
from
c6bfd07 to
38de90f
Compare
romange
changed the title
This is needed in order to eliminate cases where we return INPUT_PENDING but do not consume the whole string by rejecting just several bytes. This should simplify buffer management for the caller, so that if they pass a string that did not result in complete parsed request, at least the whole string is consumed and can be discarded. Signed-off-by: Roman Gershman <[email protected]>
src/facade/redis_parser.h
Outdated
| @@ -114,6 +114,7 @@ class RedisParser { | |||
|
|
|||
| using Blob = std::vector<uint8_t>; | |||
| std::vector<Blob> buf_stash_; | |||
| char small_buf_[32]; | |||
There was a problem hiding this comment.
Can we use std::array? and use size instead of sizeof(small_buf_)
src/facade/redis_parser.cc
Outdated
| DCHECK_LT(small_len_, 2); // must be because we never fill here with more than 2 bytes. | ||
| unsigned needed = 2 - small_len_; | ||
| unsigned consumed = min<unsigned>(needed, str.size()); | ||
|
|
||
| for (unsigned i = 0; i < consumed; ++i) { | ||
| small_buf_[small_len_++] = str[i]; | ||
| } | ||
|
|
||
| if (small_len_ < 2) { | ||
| return {INPUT_PENDING, consumed}; | ||
| } |
There was a problem hiding this comment.
feel free to ignore, it's just a small idea to make a switch case with fallthrough to avoid extra IF and DCHECK can be done in the default section
There was a problem hiding this comment.
yeah, i hate how this code looks like. and it handles only 2 chars :)
how do you suggest to do this?
There was a problem hiding this comment.
if (str.size() == 1 && small_len_ == 0) {
small_buf_[small_len_++] = str[0]; // small_len_ == 1
return {INPUT_PENDING, consumed};
}
auto str_p = str.data();
switch (small_len) {
default: assert(false);
case 0: small_buf_[small_len_++] = *(str_p++); [[fallthrough]]
case 1: small_buf_[small_len_++] = *str_p; // small_len == 2
}
After I wrote it, it doesn't look so beautiful :-)
There was a problem hiding this comment.
yeah, not much better 🤷🏼
src/facade/redis_parser.cc
Outdated
| DCHECK_EQ(bulk_len_, 0u); | ||
|
|
||
| if (bulk_len_ == 0) { |
There was a problem hiding this comment.
you have DCHECK why do you need if
Signed-off-by: Roman Gershman <[email protected]>
This change is needed in order to eliminate cases where we return INPUT_PENDING but do not consume the whole string by rejecting several bytes.
This will simplify buffer management on the caller's size, so that if they pass a string that
does not result in complete parsed request, the whole string is consumed by the parser and the passed string can be
discarded.