Skip to content

Commit 8ee2213

Browse files
committed
Code review changes.
1 parent 97d71d1 commit 8ee2213

File tree

13 files changed

+94
-96
lines changed

13 files changed

+94
-96
lines changed

include/boost/redis/config.hpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ struct config {
8888
*/
8989
std::chrono::steady_clock::duration reconnect_wait_interval = std::chrono::seconds{1};
9090

91-
/** @brief Maximum size of the read-buffer in bytes.
91+
/** @brief Maximum size of the socket read-buffer in bytes.
9292
*
9393
* Sets a limit on how much data is allowed to be read into the
9494
* read buffer. It can be used to prevent DDOS.
@@ -98,8 +98,9 @@ struct config {
9898
/** @brief read_buffer_append_size
9999
*
100100
* The size by which the read buffer grows when more space is
101-
* needed. There is no need to set this too high because memory is
102-
* reused and the growth will tend to zero.
101+
* needed. This can help avoiding some memory allocations. Once the
102+
* maximum size is reached no more memory allocations are made
103+
* since the buffer is reused.
103104
*/
104105
std::size_t read_buffer_append_size = 4096;
105106
};

include/boost/redis/connection.hpp

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -160,17 +160,19 @@ struct writer_op {
160160
template <class Conn>
161161
struct reader_op {
162162
Conn* conn_;
163+
detail::reader_fsm fsm_;
163164

164165
public:
165166
reader_op(Conn& conn) noexcept
166167
: conn_{&conn}
168+
, fsm_{conn.read_buffer_, conn.mpx_}
167169
{ }
168170

169171
template <class Self>
170172
void operator()(Self& self, system::error_code ec = {}, std::size_t n = 0)
171173
{
172174
for (;;) {
173-
auto act = conn_->read_fsm_.resume(n, ec, self.get_cancellation_state().cancelled());
175+
auto act = fsm_.resume(n, ec, self.get_cancellation_state().cancelled());
174176

175177
conn_->logger_.on_fsm_resume(act);
176178

@@ -181,7 +183,7 @@ struct reader_op {
181183
case reader_fsm::action::type::needs_more:
182184
case reader_fsm::action::type::append_some:
183185
{
184-
auto const buf = conn_->read_fsm_.get_append_buffer();
186+
auto const buf = conn_->read_buffer_.get_append_buffer();
185187
conn_->stream_.async_read_some(asio::buffer(buf), std::move(self));
186188
}
187189
return;
@@ -276,8 +278,8 @@ class run_op {
276278

277279
// If we were successful, run all the connection tasks
278280
if (!ec) {
281+
conn_->read_buffer_.clear();
279282
conn_->mpx_.reset();
280-
conn_->read_fsm_.reset();
281283

282284
// Note: Order is important here because the writer might
283285
// trigger an async_write before the async_hello thereby
@@ -385,7 +387,6 @@ class basic_connection {
385387
, reconnect_timer_{ex}
386388
, receive_channel_{ex, 256}
387389
, health_checker_{ex}
388-
, read_fsm_{mpx_}
389390
, logger_{std::move(lgr)}
390391
{
391392
set_receive_response(ignore);
@@ -489,7 +490,11 @@ class basic_connection {
489490
cfg_ = cfg;
490491
health_checker_.set_config(cfg);
491492
handshaker_.set_config(cfg);
492-
read_fsm_.set_config({cfg_.read_buffer_append_size, cfg_.max_read_size});
493+
read_buffer_.set_config({cfg.read_buffer_append_size, cfg.max_read_size});
494+
495+
// Reserve some memory to avoid excessive memory allocations in
496+
// the first reads.
497+
read_buffer_.reserve(4048u);
493498

494499
return asio::async_compose<CompletionToken, void(system::error_code)>(
495500
detail::run_op<this_type>{this},
@@ -887,8 +892,8 @@ class basic_connection {
887892
resp3_handshaker_type handshaker_;
888893

889894
config cfg_;
895+
detail::read_buffer read_buffer_;
890896
detail::multiplexer mpx_;
891-
detail::reader_fsm read_fsm_;
892897
detail::connection_logger logger_;
893898
};
894899

include/boost/redis/detail/multiplexer.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
#include <boost/redis/resp3/type.hpp>
1515
#include <boost/redis/usage.hpp>
1616

17-
#include <boost/system.hpp>
17+
#include <boost/system/error_code.hpp>
1818

1919
#include <algorithm>
2020
#include <deque>

include/boost/redis/detail/read_buffer.hpp

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,26 +8,33 @@
88
#define BOOST_REDIS_READ_BUFFER_HPP
99

1010
#include <boost/core/span.hpp>
11+
#include <boost/system/error_code.hpp>
1112

1213
#include <cstddef>
13-
#include <vector>
1414
#include <string_view>
1515
#include <utility>
16+
#include <vector>
1617

1718
namespace boost::redis::detail {
1819

1920
class read_buffer {
2021
public:
2122
using span_type = span<char>;
2223

23-
[[nodiscard]]
24-
system::error_code prepare_append(std::size_t append_size, std::size_t max_buffer_size);
24+
// See config.hpp for the meaning of these parameters.
25+
struct config {
26+
std::size_t read_buffer_append_size = 4096u;
27+
std::size_t max_read_size = static_cast<std::size_t>(-1);
28+
};
2529

26-
void commit_append(std::size_t read_size);
30+
[[nodiscard]]
31+
auto prepare_append() -> system::error_code;
2732

2833
[[nodiscard]]
2934
auto get_append_buffer() noexcept -> span_type;
3035

36+
void commit_append(std::size_t read_size);
37+
3138
[[nodiscard]]
3239
auto get_committed_buffer() const noexcept -> std::string_view;
3340

@@ -41,13 +48,14 @@ class read_buffer {
4148

4249
void reserve(std::size_t n);
4350

44-
friend
45-
bool operator==(read_buffer const& lhs, read_buffer const& rhs);
51+
friend bool operator==(read_buffer const& lhs, read_buffer const& rhs);
52+
53+
friend bool operator!=(read_buffer const& lhs, read_buffer const& rhs);
4654

47-
friend
48-
bool operator!=(read_buffer const& lhs, read_buffer const& rhs);
55+
void set_config(config const& cfg) noexcept { cfg_ = cfg; };
4956

5057
private:
58+
config cfg_ = config{};
5159
std::vector<char> buffer_;
5260
std::size_t append_buf_begin_ = 0;
5361
};

include/boost/redis/detail/reader_fsm.hpp

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66

77
#ifndef BOOST_REDIS_READER_FSM_HPP
88
#define BOOST_REDIS_READER_FSM_HPP
9-
109
#include <boost/redis/detail/multiplexer.hpp>
1110

1211
#include <boost/asio/cancellation_type.hpp>
@@ -16,14 +15,10 @@
1615

1716
namespace boost::redis::detail {
1817

18+
class read_buffer;
19+
1920
class reader_fsm {
2021
public:
21-
// See config.hpp for the meaning of these parameters.
22-
struct config {
23-
std::size_t read_buffer_append_size = 4096;
24-
std::size_t max_read_size = -1;
25-
};
26-
2722
struct action {
2823
enum class type
2924
{
@@ -36,31 +31,20 @@ class reader_fsm {
3631
};
3732

3833
type type_ = type::setup_cancellation;
39-
std::size_t push_size_ = 0;
34+
std::size_t push_size_ = 0u;
4035
system::error_code ec_ = {};
4136
};
4237

43-
explicit reader_fsm(multiplexer& mpx) noexcept;
38+
explicit reader_fsm(read_buffer& rbuf, multiplexer& mpx) noexcept;
4439

4540
action resume(
4641
std::size_t bytes_read,
4742
system::error_code ec,
4843
asio::cancellation_type_t /*cancel_state*/);
4944

50-
void set_config(config const& cfg) noexcept { cfg_ = cfg; };
51-
52-
void reset();
53-
54-
[[nodiscard]]
55-
auto get_append_buffer() noexcept
56-
{
57-
return read_buffer_.get_append_buffer();
58-
}
59-
6045
private:
6146
int resume_point_{0};
62-
read_buffer read_buffer_;
63-
config cfg_;
47+
read_buffer* read_buffer_ = nullptr;
6448
action action_after_resume_;
6549
action::type next_read_type_ = action::type::append_some;
6650
multiplexer* mpx_ = nullptr;

include/boost/redis/error.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ enum class error
8989
/// The configuration specified UNIX sockets with SSL, which is not supported.
9090
unix_sockets_ssl_unsupported,
9191

92-
/// The size of the read buffer would exceed it maximum configured value.
92+
/// Reading data from the socket would exceed the maximum size allowed of the read buffer.
9393
exceeds_maximum_read_buffer_size,
9494
};
9595

include/boost/redis/impl/error.ipp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ struct error_category_impl : system::error_category {
5151
case error::unix_sockets_ssl_unsupported:
5252
return "The configuration specified UNIX sockets with SSL, which is not supported.";
5353
case error::exceeds_maximum_read_buffer_size:
54-
return "The size of the read buffer would exceed it maximum configured value";
54+
return "Reading data from the socket would exceed the maximum size allowed of the read "
55+
"buffer.";
5556
default: BOOST_ASSERT(false); return "Boost.Redis error.";
5657
}
5758
}

include/boost/redis/impl/read_buffer.ipp

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,13 @@
1313

1414
namespace boost::redis::detail {
1515

16-
system::error_code
17-
read_buffer::prepare_append(std::size_t append_size, std::size_t max_buffer_size)
16+
system::error_code read_buffer::prepare_append()
1817
{
1918
BOOST_ASSERT(append_buf_begin_ == buffer_.size());
2019

21-
auto const new_size = append_buf_begin_ + append_size;
20+
auto const new_size = append_buf_begin_ + cfg_.read_buffer_append_size;
2221

23-
if (new_size > max_buffer_size) {
22+
if (new_size > cfg_.max_read_size) {
2423
return error::exceeds_maximum_read_buffer_size;
2524
}
2625

@@ -57,7 +56,8 @@ void read_buffer::clear()
5756

5857
std::size_t read_buffer::consume_committed(std::size_t size)
5958
{
60-
// Consumes only committed data.
59+
// For convenience, if the requested size is larger than the
60+
// committed buffer we cap it to the maximum.
6161
if (size > append_buf_begin_)
6262
size = append_buf_begin_;
6363

@@ -67,21 +67,13 @@ std::size_t read_buffer::consume_committed(std::size_t size)
6767
return size;
6868
}
6969

70-
void read_buffer::reserve(std::size_t n)
71-
{
72-
buffer_.reserve(n);
73-
}
70+
void read_buffer::reserve(std::size_t n) { buffer_.reserve(n); }
7471

7572
bool operator==(read_buffer const& lhs, read_buffer const& rhs)
7673
{
77-
return
78-
lhs.buffer_ == rhs.buffer_ &&
79-
lhs.append_buf_begin_ == rhs.append_buf_begin_;
74+
return lhs.buffer_ == rhs.buffer_ && lhs.append_buf_begin_ == rhs.append_buf_begin_;
8075
}
8176

82-
bool operator!=(read_buffer const& lhs, read_buffer const& rhs)
83-
{
84-
return !(lhs == rhs);
85-
}
77+
bool operator!=(read_buffer const& lhs, read_buffer const& rhs) { return !(lhs == rhs); }
8678

8779
} // namespace boost::redis::detail

include/boost/redis/impl/reader_fsm.ipp

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@
66

77
#include <boost/redis/detail/coroutine.hpp>
88
#include <boost/redis/detail/multiplexer.hpp>
9+
#include <boost/redis/detail/read_buffer.hpp>
910
#include <boost/redis/detail/reader_fsm.hpp>
1011

1112
namespace boost::redis::detail {
1213

13-
reader_fsm::reader_fsm(multiplexer& mpx) noexcept
14-
: mpx_{&mpx}
14+
reader_fsm::reader_fsm(read_buffer& rbuf, multiplexer& mpx) noexcept
15+
: read_buffer_{&rbuf}
16+
, mpx_{&mpx}
1517
{ }
1618

1719
reader_fsm::action reader_fsm::resume(
@@ -24,15 +26,15 @@ reader_fsm::action reader_fsm::resume(
2426
BOOST_REDIS_YIELD(resume_point_, 1, action::type::setup_cancellation)
2527

2628
for (;;) {
27-
ec = read_buffer_.prepare_append(cfg_.read_buffer_append_size, cfg_.max_read_size);
29+
ec = read_buffer_->prepare_append();
2830
if (ec) {
2931
action_after_resume_ = {action::type::done, 0, ec};
3032
BOOST_REDIS_YIELD(resume_point_, 2, action::type::cancel_run)
3133
return action_after_resume_;
3234
}
3335

3436
BOOST_REDIS_YIELD(resume_point_, 3, next_read_type_)
35-
read_buffer_.commit_append(bytes_read);
37+
read_buffer_->commit_append(bytes_read);
3638
if (ec) {
3739
// TODO: If an error occurred but data was read (i.e.
3840
// bytes_read != 0) we should try to process that data and
@@ -43,8 +45,8 @@ reader_fsm::action reader_fsm::resume(
4345
}
4446

4547
next_read_type_ = action::type::append_some;
46-
while (read_buffer_.get_committed_size() != 0) {
47-
res_ = mpx_->consume_next(read_buffer_.get_committed_buffer(), ec);
48+
while (read_buffer_->get_committed_size() != 0) {
49+
res_ = mpx_->consume_next(read_buffer_->get_committed_buffer(), ec);
4850
if (ec) {
4951
// TODO: Perhaps log what has not been consumed to aid
5052
// debugging.
@@ -58,7 +60,7 @@ reader_fsm::action reader_fsm::resume(
5860
break;
5961
}
6062

61-
read_buffer_.consume_committed(res_.second);
63+
read_buffer_->consume_committed(res_.second);
6264

6365
if (res_.first.value()) {
6466
BOOST_REDIS_YIELD(resume_point_, 6, action::type::notify_push_receiver, res_.second)
@@ -83,12 +85,4 @@ reader_fsm::action reader_fsm::resume(
8385
return {action::type::done, 0, system::error_code()};
8486
}
8587

86-
void reader_fsm::reset()
87-
{
88-
resume_point_ = 0;
89-
next_read_type_ = action::type::append_some;
90-
res_ = {std::make_pair(std::nullopt, 0)};
91-
read_buffer_.clear();
92-
}
93-
9488
} // namespace boost::redis::detail

test/common.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,9 @@ void run_coroutine_test(net::awaitable<void> op, std::chrono::steady_clock::dura
6969
}
7070
#endif // BOOST_ASIO_HAS_CO_AWAIT
7171

72-
void append_read_data(boost::redis::detail::reader_fsm& fsm, std::string_view data)
72+
void append_read_data(boost::redis::detail::read_buffer& rbuf, std::string_view data)
7373
{
74-
auto const buffer = fsm.get_append_buffer();
74+
auto const buffer = rbuf.get_append_buffer();
7575
BOOST_ASSERT(data.size() <= buffer.size());
7676
std::copy(data.begin(), data.end(), buffer.begin());
7777
}

0 commit comments

Comments
 (0)