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.

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

Use QuicheMemSlice constructor with releasor. #31167

Merged
merged 10 commits into from
Dec 12, 2023
1 change: 1 addition & 0 deletions source/common/buffer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ envoy_cc_library(
"//source/common/common:non_copyable",
"//source/common/common:utility_lib",
"//source/common/event:libevent_lib",
"@com_google_absl//absl/functional:any_invocable",
],
)

Expand Down
8 changes: 5 additions & 3 deletions source/common/buffer/buffer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "source/common/common/utility.h"
#include "source/common/event/libevent.h"

#include "absl/functional/any_invocable.h"

namespace Envoy {
namespace Buffer {

Expand Down Expand Up @@ -604,8 +606,8 @@ class BufferFragmentImpl : NonCopyable, public BufferFragment {
*/
BufferFragmentImpl(
const void* data, size_t size,
const std::function<void(const void*, size_t, const BufferFragmentImpl*)>& releasor)
: data_(data), size_(size), releasor_(releasor) {}
absl::AnyInvocable<void(const void*, size_t, const BufferFragmentImpl*)> releasor)
: data_(data), size_(size), releasor_(std::move(releasor)) {}

// Buffer::BufferFragment
const void* data() const override { return data_; }
Expand All @@ -619,7 +621,7 @@ class BufferFragmentImpl : NonCopyable, public BufferFragment {
private:
const void* const data_;
const size_t size_;
const std::function<void(const void*, size_t, const BufferFragmentImpl*)> releasor_;
absl::AnyInvocable<void(const void*, size_t, const BufferFragmentImpl*)> releasor_;
};

class LibEventInstance : public Instance {
Expand Down
14 changes: 13 additions & 1 deletion source/common/quic/envoy_quic_client_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,19 @@ void EnvoyQuicClientStream::encodeData(Buffer::Instance& data, bool end_stream)
// TODO(danzh): investigate the cost of allocating one buffer per slice.
// If it turns out to be expensive, add a new function to free data in the middle in buffer
// interface and re-design QuicheMemSliceImpl.
quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_);
if (!Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.quiche_use_mem_slice_releasor_api")) {
quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_);
} else {
auto single_slice_buffer = std::make_unique<Buffer::OwnedImpl>();
single_slice_buffer->move(data, slice.len_);
quic_slices.emplace_back(
reinterpret_cast<char*>(single_slice_buffer->frontSlice().mem_), slice.len_,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bazel -c opt --config=gcc seems to complain about nullptr here. I guess this may be due to an issue with function parameter evaluation order?

It's likely safer to just use slice.mem_ here. (That may or may not be the segfault / crash I'm seeing.)

[single_slice_buffer = std::move(single_slice_buffer)](const char*) mutable {
// Free this memory explicitly when the callback is invoked.
single_slice_buffer = nullptr;
});
}
}
quic::QuicConsumedData result{0, false};
absl::Span<quiche::QuicheMemSlice> span(quic_slices);
Expand Down
14 changes: 13 additions & 1 deletion source/common/quic/envoy_quic_server_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,19 @@ void EnvoyQuicServerStream::encodeData(Buffer::Instance& data, bool end_stream)
// TODO(danzh): investigate the cost of allocating one buffer per slice.
// If it turns out to be expensive, add a new function to free data in the middle in buffer
// interface and re-design QuicheMemSliceImpl.
quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_);
if (!Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.quiche_use_mem_slice_releasor_api")) {
quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_);
} else {
auto single_slice_buffer = std::make_unique<Buffer::OwnedImpl>();
single_slice_buffer->move(data, slice.len_);
quic_slices.emplace_back(
reinterpret_cast<char*>(single_slice_buffer->frontSlice().mem_), slice.len_,
[single_slice_buffer = std::move(single_slice_buffer)](const char*) mutable {
// Free this memory explicitly when the callback is invoked.
single_slice_buffer = nullptr;
});
}
}
quic::QuicConsumedData result{0, false};
absl::Span<quiche::QuicheMemSlice> span(quic_slices);
Expand Down
4 changes: 3 additions & 1 deletion source/common/quic/platform/quiche_mem_slice_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ QuicheMemSliceImpl::QuicheMemSliceImpl(std::unique_ptr<char[]> buffer, size_t le
QuicheMemSliceImpl::QuicheMemSliceImpl(char buffer[], size_t length,
SingleUseCallback<void(const char*)> deleter)
: fragment_(std::make_unique<Envoy::Buffer::BufferFragmentImpl>(
buffer, length, [&](const void* p, size_t, const Envoy::Buffer::BufferFragmentImpl*) {
buffer, length,
[deleter = std::move(deleter)](const void* p, size_t,
const Envoy::Buffer::BufferFragmentImpl*) mutable {
std::move(deleter)(reinterpret_cast<const char*>(p));
})) {
single_slice_buffer_.addBufferFragment(*fragment_);
Expand Down
2 changes: 2 additions & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);
FALSE_RUNTIME_GUARD(envoy_reloadable_features_refresh_rtt_after_request);
// TODO(danzh) false deprecate it once QUICHE has its own enable/disable flag.
FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_reject_all);
// TODO(steveWang) flip this to true after this is verified in prod.
FALSE_RUNTIME_GUARD(envoy_reloadable_features_quiche_use_mem_slice_releasor_api);
// TODO(suniltheta): Once the newly added http async technique is stabilized move it under
// RUNTIME_GUARD so that this option becomes default enabled. Once this option proves effective
// remove the feature flag and remove code path that relies on old technique to fetch credentials
Expand Down
2 changes: 2 additions & 0 deletions test/common/quic/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ envoy_cc_test(
"//test/mocks/http:http_mocks",
"//test/mocks/http:stream_decoder_mock",
"//test/mocks/network:network_mocks",
"//test/test_common:test_runtime_lib",
"//test/test_common:utility_lib",
"@com_github_google_quiche//:quic_core_http_spdy_session_lib",
"@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib",
Expand All @@ -148,6 +149,7 @@ envoy_cc_test(
"//test/mocks/http:http_mocks",
"//test/mocks/http:stream_decoder_mock",
"//test/mocks/network:network_mocks",
"//test/test_common:test_runtime_lib",
"//test/test_common:utility_lib",
"@com_github_google_quiche//:quic_core_http_spdy_session_lib",
"@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib",
Expand Down
32 changes: 32 additions & 0 deletions test/common/quic/envoy_quic_client_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "test/mocks/http/mocks.h"
#include "test/mocks/http/stream_decoder.h"
#include "test/mocks/network/mocks.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
Expand Down Expand Up @@ -247,6 +248,37 @@ TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponse) {
quic_stream_->OnStreamFrame(frame);
}

TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponseWithMemSliceReleasor) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.quiche_use_mem_slice_releasor_api", "true"}});

EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions());
const auto result = quic_stream_->encodeHeaders(request_headers_, false);
EXPECT_TRUE(result.ok());
quic_stream_->encodeData(request_body_, false);
quic_stream_->encodeTrailers(request_trailers_);

size_t offset = receiveResponse(response_body_, false);
EXPECT_CALL(stream_decoder_, decodeTrailers_(_))
.WillOnce(Invoke([](const Http::ResponseTrailerMapPtr& headers) {
Http::LowerCaseString key1("key1");
Http::LowerCaseString key2(":final-offset");
EXPECT_EQ("value1", headers->get(key1)[0]->value().getStringView());
EXPECT_TRUE(headers->get(key2).empty());
}));
std::string more_response_body{"bbb"};
EXPECT_CALL(stream_decoder_, decodeData(_, _))
.WillOnce(Invoke([&](Buffer::Instance& buffer, bool finished_reading) {
EXPECT_EQ(more_response_body, buffer.toString());
EXPECT_EQ(false, finished_reading);
}));
std::string payload = absl::StrCat(bodyToHttp3StreamPayload(more_response_body),
spdyHeaderToHttp3StreamPayload(spdy_trailers_));
quic::QuicStreamFrame frame(stream_id_, true, offset, payload);
quic_stream_->OnStreamFrame(frame);
}

TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponseWithAccounting) {
EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions());
EXPECT_EQ(0, quic_stream_->bytesMeter()->wireBytesSent());
Expand Down
16 changes: 16 additions & 0 deletions test/common/quic/envoy_quic_server_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "test/mocks/http/mocks.h"
#include "test/mocks/http/stream_decoder.h"
#include "test/mocks/network/mocks.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/test_time.h"
#include "test/test_common/utility.h"

Expand Down Expand Up @@ -278,6 +279,21 @@ TEST_F(EnvoyQuicServerStreamTest, PostRequestAndResponse) {
EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions());
receiveRequest(request_body_, true, request_body_.size() * 2);
quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false);

std::string response(18 * 1024, 'a');
Buffer::OwnedImpl buffer(response);
quic_stream_->encodeData(buffer, false);
quic_stream_->encodeTrailers(response_trailers_);
}

TEST_F(EnvoyQuicServerStreamTest, PostRequestAndResponseWithMemSliceReleasor) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.quiche_use_mem_slice_releasor_api", "true"}});

EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions());
receiveRequest(request_body_, true, request_body_.size() * 2);
quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false);
quic_stream_->encodeTrailers(response_trailers_);
}

Expand Down
Loading