feat(resp): optimize simple string "OK" construction #2627
Conversation
RiversJin
force-pushed
the
opt_simple_ok
branch
3 times, most recently
from
416b182 to
a2a8953
Compare
src/server/redis_reply.cc
Outdated
| std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisErrorMsg(s) + CRLF; } | ||
| std::string Error(const Status &s) { | ||
| std::string res; | ||
| res.reserve(s.Msg().size() + 3); // 1 for '-', 2 for CRLF |
There was a problem hiding this comment.
I wonder that s.Msg() might not be really the final string generation when redisErrorPrefixMapping in StatusToRedisErrorMsg contains the specific error message.
There was a problem hiding this comment.
Yes, you're absolutely right; I overlooked the possibility that the text could come directly from redisErrorPrefixMapping. However, performing a hash lookup here to confirm the reserved length might be a bit heavy.
Or, do you think it’s a good idea to directly use std::max(16, s.Msg().size() + 3)? In most cases, the minimum allocation unit by the memory allocator is often two pointer lengths, and for the contents within redisErrorPrefixMapping, 16 bytes should be sufficient.
There was a problem hiding this comment.
However, performing a hash lookup here to confirm the reserved length might be a bit heavy.
As the hash table is small and anyway it would be built 😅, I think construct error string firstly is ok...
do you think it’s a good idea to directly use std::max(16, s.Msg().size() + 3)
It would be good if it's a critical path, but I don't think the error path worth this, since the current code is clear to understand and Error path might not a bottleneck...
There was a problem hiding this comment.
Ok, I've reverted this modification.
RiversJin
force-pushed
the
opt_simple_ok
branch
2 times, most recently
from
043e113 to
dff425f
Compare
|
Would you mind fix the lint? |
RiversJin
force-pushed
the
opt_simple_ok
branch
2 times, most recently
from
048fa6e to
c58a7ff
Compare
Sure but ... Since std::string_view cannot be implicitly converted to std::string, and I want to implement RESP_OK as a constexpr variable, I changed it to use constexpr char[]. |
This not fixes the problem, we can either use |
RiversJin
force-pushed
the
opt_simple_ok
branch
from
c58a7ff to
8212747
Compare
Ok u're right, I've done |
src/server/redis_reply.h
Outdated
| std::string SimpleString(const std::string &data); | ||
| std::string SimpleString(std::string_view data); | ||
|
|
||
| static inline std::string RESP_OK = RESP_PREFIX_SIMPLE_STRING "OK" CRLF; |
… pre-allocated memory By pre-allocating memory, the speed rate of simple strings can be improved. Additionally, using a predefined "+OK\r\n" string eliminates the need for string concatenation when returning "OK".
RiversJin
force-pushed
the
opt_simple_ok
branch
from
8212747 to
306c2f7
Compare
PragmaTwice
changed the title
|
… pre-allocated memory
By pre-allocating memory, the speed rate of simple strings can be improved. Additionally, using a predefined "+OK\r\n" string eliminates the need for string concatenation when returning "OK".
It solves #2626