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.

Sign up for GitHub

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
Merged

Use QuicheMemSlice constructor with releasor. #31167

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
70dd810
Use QuicheMemSlice constructor with releasor.
steveWang Dec 4, 2023
4d47164
Remove names for unused parameters.
steveWang Dec 4, 2023
0f8f5fe
Fix use-after-stack in QuicheMemSliceImpl.
steveWang Dec 4, 2023
d9fa5b7
Add runtime and compile-time guard.
steveWang Dec 6, 2023
297eea0
Remove ifdef guard.
steveWang Dec 7, 2023
19aa02f
Clean up memslice storage on calling releasor.
steveWang Dec 8, 2023
f7863c4
Add tests enabling releasor API runtime guard.
steveWang Dec 8, 2023
9736fa4
Call encodeData in envoy_quic_server_stream_test.
steveWang Dec 8, 2023
42a1222
Merge branch 'main' into quiche-buffer
steveWang Dec 8, 2023
44af392
Address use-after-move of single_slice_buffer.
steveWang Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/common/buffer/BUILD
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 5 additions & 1 deletion source/common/quic/envoy_quic_client_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ 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_);
auto single_slice_buffer = std::make_unique<Buffer::OwnedImpl>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This change I would love a way to fallback in case it goes wrong. RUNTIME_GUARD isn't an option here because we want to compile out the old way internally. Would #ifdef SOME_PREPROCESSOR be acceptable? @alyssawilk

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to compile out the old way overnight? Can we do the usual flip-wait-deprecate?

Copy link
Contributor

Choose a reason for hiding this comment

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

flip-wait-deprecate takes 6 months, and preferably we don't want to wait for that long.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah so for functional changes you need to wait 6 months for operators to update and test, but for "I want to guard this in case it causes crashes but really things look fine" changes you can remove at your own pace. Does that help any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Dan mentions, we'd like to be able to change the compile-time dependencies (in particular so we no longer depend on a custom constructor in Envoy platform's QuicheMemSliceImpl).

I'm happy to leave the default behavior as-is and protect this new behavior behind a compile-time guard so everyone who doesn't care about swapping out this dependency is unaffected, but guarding it behind a runtime flag means we can't actually swap out the underlying implementation until the old way is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how we get a fallback and keep the code in, so I was trying to offer a less painful timeline to get both in sequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I don't think a runtime fallback is feasible for what I want to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RyanTheOptimist FYI. I think we want to use a runtime guard with an ifdef. Something like...

#ifdef USE_QUICHE_MEM_SLICE_RELEASOR_API_EXCLUSIVELY
if (!RUNTIME_GUARD(use_quiche_mem_slice_releasor)) {
  quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_);
} else {
#endif
  auto single_slice_buffer = std::make_unique<Buffer::OwnedImpl>();
  // ...
#ifdef USE_QUICHE_MEM_SLICE_RELEASOR_API_EXCLUSIVELY
}
#endif

(Rough pseudocode. I'll send a commit along these lines later today.)

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*) {});
}
quic::QuicConsumedData result{0, false};
absl::Span<quiche::QuicheMemSlice> span(quic_slices);
Expand Down
6 changes: 5 additions & 1 deletion source/common/quic/envoy_quic_server_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,11 @@ 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_);
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*) {});
}
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
View file
Open in desktop
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
Loading