From 2a9e3e607bd64b64ff6264616e3078b39d67f050 Mon Sep 17 00:00:00 2001 From: adi_holden Date: Thu, 6 Mar 2025 16:10:32 +0200 Subject: [PATCH 1/2] Revert "chore: minor clean ups before introducing ProvidedBuffers (#4709)" This reverts commit a39d777b82138bcafb1b67c09927ef3f166755f9. --- .vscode/c_cpp_properties.json | 30 +++++++++++++++--------------- src/facade/dragonfly_connection.cc | 22 ++++++++-------------- src/facade/facade_test.cc | 2 +- src/facade/redis_parser.cc | 18 +++++++++--------- src/facade/resp_expr.h | 4 ++-- 5 files changed, 35 insertions(+), 41 deletions(-) diff --git a/.vscode/c_cpp_properties.json b/.vscode/c_cpp_properties.json index 65106eafc306..ed591a1652d3 100644 --- a/.vscode/c_cpp_properties.json +++ b/.vscode/c_cpp_properties.json @@ -1,16 +1,16 @@ { - "configurations": [ - { - "name": "Linux", - "includePath": [ - "${default}" - ], - "cStandard": "c17", - "cppStandard": "c++17", - "intelliSenseMode": "${default}", - "compileCommands": "${workspaceFolder}/build-dbg/compile_commands.json", - "configurationProvider": "ms-vscode.cmake-tools" - } - ], - "version": 4 -} + "configurations": [ + { + "name": "Linux", + "includePath": [ + "${default}" + ], + "cStandard": "c17", + "cppStandard": "c++17", + "intelliSenseMode": "${default}", + "compileCommands": "${workspaceFolder}/build-dbg/compile_commands.json", + "configurationProvider": "ms-vscode.cmake-tools" + } + ], + "version": 4 +} \ No newline at end of file diff --git a/src/facade/dragonfly_connection.cc b/src/facade/dragonfly_connection.cc index e1d4c3f97de9..a9cde22703ec 100644 --- a/src/facade/dragonfly_connection.cc +++ b/src/facade/dragonfly_connection.cc @@ -1150,11 +1150,8 @@ Connection::ParserStatus Connection::ParseRedis() { return {FromArgs(std::move(parse_args), tlh)}; }; - RedisParser::Buffer input_buf = io_buf_.InputBuffer(); - size_t available_to_parse = io_buf_.InputLen(); - do { - result = redis_parser_->Parse(input_buf, &consumed, &parse_args); + result = redis_parser_->Parse(io_buf_.InputBuffer(), &consumed, &parse_args); request_consumed_bytes_ += consumed; if (result == RedisParser::OK && !parse_args.empty()) { if (RespExpr& first = parse_args.front(); first.type == RespExpr::STRING) @@ -1163,7 +1160,7 @@ Connection::ParserStatus Connection::ParseRedis() { if (io_req_size_hist) io_req_size_hist->Add(request_consumed_bytes_); request_consumed_bytes_ = 0; - bool has_more = consumed < available_to_parse; + bool has_more = consumed < io_buf_.InputLen(); if (tl_traffic_logger.log_file && IsMain() /* log only on the main interface */) { LogTraffic(id_, has_more, absl::MakeSpan(parse_args), service_->GetContextInfo(cc_.get())); @@ -1171,11 +1168,9 @@ Connection::ParserStatus Connection::ParseRedis() { DispatchSingle(has_more, dispatch_sync, dispatch_async); } - available_to_parse -= consumed; - input_buf.remove_prefix(consumed); - } while (RedisParser::OK == result && available_to_parse > 0 && !reply_builder_->GetError()); + io_buf_.ConsumeInput(consumed); + } while (RedisParser::OK == result && io_buf_.InputLen() > 0 && !reply_builder_->GetError()); - io_buf_.ConsumeInput(io_buf_.InputLen()); parser_error_ = result; if (result == RedisParser::OK) return OK; @@ -1183,8 +1178,6 @@ Connection::ParserStatus Connection::ParseRedis() { if (result == RedisParser::INPUT_PENDING) return NEED_MORE; - VLOG(1) << "Parser error " << result; - return ERROR; } @@ -1312,11 +1305,12 @@ void Connection::HandleMigrateRequest() { } error_code Connection::HandleRecvSocket() { - phase_ = READ_SOCKET; - io::MutableBytes append_buf = io_buf_.AppendBuffer(); DCHECK(!append_buf.empty()); + phase_ = READ_SOCKET; + error_code ec; + ::io::Result recv_sz = socket_->Recv(append_buf); last_interaction_ = time(nullptr); @@ -1330,7 +1324,7 @@ error_code Connection::HandleRecvSocket() { stats_->io_read_bytes += commit_sz; ++stats_->io_read_cnt; - return {}; + return ec; } auto Connection::IoLoop() -> variant { diff --git a/src/facade/facade_test.cc b/src/facade/facade_test.cc index 70de323ad11b..a20b2667fa26 100644 --- a/src/facade/facade_test.cc +++ b/src/facade/facade_test.cc @@ -34,7 +34,7 @@ bool RespMatcher::MatchAndExplain(RespExpr e, MatchResultListener* listener) con if (type_ == RespExpr::STRING || type_ == RespExpr::ERROR) { RespExpr::Buffer ebuf = e.GetBuf(); - std::string_view actual{reinterpret_cast(ebuf.data()), ebuf.size()}; + std::string_view actual{reinterpret_cast(ebuf.data()), ebuf.size()}; if (type_ == RespExpr::ERROR && !absl::StrContains(actual, exp_str_)) { *listener << "Actual does not contain '" << exp_str_ << "'"; diff --git a/src/facade/redis_parser.cc b/src/facade/redis_parser.cc index 09174d9f70be..072d40c8fcd1 100644 --- a/src/facade/redis_parser.cc +++ b/src/facade/redis_parser.cc @@ -185,11 +185,11 @@ void RedisParser::StashState(RespExpr::Vec* res) { auto RedisParser::ParseInline(Buffer str) -> ResultConsumed { DCHECK(!str.empty()); - const uint8_t* ptr = str.begin(); - const uint8_t* end = str.end(); - const uint8_t* token_start = ptr; + uint8_t* ptr = str.begin(); + uint8_t* end = str.end(); + uint8_t* token_start = ptr; - auto find_token_end = [](const uint8_t* ptr, const uint8_t* end) { + auto find_token_end = [](uint8_t* ptr, uint8_t* end) { while (ptr != end && *ptr > 32) ++ptr; return ptr; @@ -411,8 +411,8 @@ auto RedisParser::ParseArg(Buffer str) -> ResultConsumed { return ConsumeArrayLen(str); } - const char* s = reinterpret_cast(str.data()); - const char* eol = reinterpret_cast(memchr(s, '\n', str.size())); + char* s = reinterpret_cast(str.data()); + char* eol = reinterpret_cast(memchr(s, '\n', str.size())); // TODO: in client mode we still may not consume everything (see INPUT_PENDING below). // It's not a problem, because we need consume all the input only in server mode. @@ -428,7 +428,7 @@ auto RedisParser::ParseArg(Buffer str) -> ResultConsumed { return {BAD_STRING, 0}; cached_expr_->emplace_back(arg_c_ == '+' ? RespExpr::STRING : RespExpr::ERROR); - cached_expr_->back().u = Buffer{reinterpret_cast(s), size_t((eol - 1) - s)}; + cached_expr_->back().u = Buffer{reinterpret_cast(s), size_t((eol - 1) - s)}; } else if (arg_c_ == ':') { DCHECK(!server_mode_); if (!eol) { @@ -477,7 +477,7 @@ auto RedisParser::ConsumeBulk(Buffer str) -> ResultConsumed { // is_broken_token_ can be false, if we just parsed the bulk length but have // not parsed the token itself. if (is_broken_token_) { - memcpy(const_cast(bulk_str.end()), str.data(), bulk_len_); + memcpy(bulk_str.end(), str.data(), bulk_len_); bulk_str = Buffer{bulk_str.data(), bulk_str.size() + bulk_len_}; } else { bulk_str = str.subspan(0, bulk_len_); @@ -506,7 +506,7 @@ auto RedisParser::ConsumeBulk(Buffer str) -> ResultConsumed { size_t len = std::min(str.size(), bulk_len_); if (is_broken_token_) { - memcpy(const_cast(bulk_str.end()), str.data(), len); + memcpy(bulk_str.end(), str.data(), len); bulk_str = Buffer{bulk_str.data(), bulk_str.size() + len}; DVLOG(1) << "Extending bulk stash to size " << bulk_str.size(); } else { diff --git a/src/facade/resp_expr.h b/src/facade/resp_expr.h index 52725e993acf..43b940783f6e 100644 --- a/src/facade/resp_expr.h +++ b/src/facade/resp_expr.h @@ -18,7 +18,7 @@ namespace facade { class RespExpr { public: - using Buffer = absl::Span; + using Buffer = absl::Span; enum Type : uint8_t { STRING, ARRAY, INT64, DOUBLE, NIL, NIL_ARRAY, ERROR }; @@ -70,7 +70,7 @@ using RespVec = RespExpr::Vec; using RespSpan = absl::Span; inline std::string_view ToSV(RespExpr::Buffer buf) { - return std::string_view{reinterpret_cast(buf.data()), buf.size()}; + return std::string_view{reinterpret_cast(buf.data()), buf.size()}; } } // namespace facade From cef47ccf6e81538eca932995d8b439bb27a3ea3c Mon Sep 17 00:00:00 2001 From: adi_holden Date: Thu, 6 Mar 2025 17:56:15 +0200 Subject: [PATCH 2/2] unrevert vscode Signed-off-by: adi_holden --- .vscode/c_cpp_properties.json | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/.vscode/c_cpp_properties.json b/.vscode/c_cpp_properties.json index ed591a1652d3..65106eafc306 100644 --- a/.vscode/c_cpp_properties.json +++ b/.vscode/c_cpp_properties.json @@ -1,16 +1,16 @@ { - "configurations": [ - { - "name": "Linux", - "includePath": [ - "${default}" - ], - "cStandard": "c17", - "cppStandard": "c++17", - "intelliSenseMode": "${default}", - "compileCommands": "${workspaceFolder}/build-dbg/compile_commands.json", - "configurationProvider": "ms-vscode.cmake-tools" - } - ], - "version": 4 -} \ No newline at end of file + "configurations": [ + { + "name": "Linux", + "includePath": [ + "${default}" + ], + "cStandard": "c17", + "cppStandard": "c++17", + "intelliSenseMode": "${default}", + "compileCommands": "${workspaceFolder}/build-dbg/compile_commands.json", + "configurationProvider": "ms-vscode.cmake-tools" + } + ], + "version": 4 +}