buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque#14282
Conversation
…g subclassing and storing slice info directly in the SliceDeque Signed-off-by: Antonio Vicente <avd@google.com>
test/common/buffer/buffer_test.cc
Outdated
| deletion_callback_(); | ||
| } | ||
| } | ||
| TEST(SliceTest, OwnedMoveConstruction) { |
There was a problem hiding this comment.
TODO: change to TEST_P
| } | ||
|
|
||
| Slice(Slice&& rhs) noexcept { | ||
| storage_ = std::move(rhs.storage_); |
There was a problem hiding this comment.
TODO: Use helper function to code shared between move constructor and move assignment?
Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
I like the approach. This will be much easier/smaller to review without the snake-to-camel case name change though.
/wait
Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
…tor_v2 Signed-off-by: Antonio Vicente <avd@google.com>
| // size_t instead of uint64_t in the Slice interface. | ||
| raw_slices.emplace_back(RawSlice{slice->data(), static_cast<size_t>(slice->dataSize())}); | ||
| raw_slices.emplace_back( | ||
| RawSlice{const_cast<uint8_t*>(slice.data()), static_cast<size_t>(slice.dataSize())}); |
There was a problem hiding this comment.
Would it be worth creating a ConstRawSliceVector to avoid the const_cast? I think uses of getRawSlices() do not modify the slice?
There was a problem hiding this comment.
The introduction of ConstRawSlice in parallel to RawSlice will require API across multiple interfaces. Some examples include arguments to IoHandle::writev which should use ConstRawSlice, while readv should continue using RawSlice. Some const casts would be needed when you have ConstRawSlice and are feeding that input into C APIs like nghttp2_hd_inflate_hd2 in MetadataDecoder::decodeMetadataPayloadUsingNghttp2
This PR already feels huge. Happy to try the change as a followup if you'ld like.
There was a problem hiding this comment.
Yeah, agree we shouldn't expand the scope that much in this PR.
* master: buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque (envoyproxy#14282) gRPC client to be used by ext_proc filter (envoyproxy#14283) http2: Add integration tests for PRIORITY frame flood mitigation for upstream servers (envoyproxy#14328) event: touch watchdog before execution of each post callback and before deferred deletion (envoyproxy#14339) stale: more allowed ops (envoyproxy#14345) stale: more changes (envoyproxy#14344) test: TODO fixup making enable_half_close private envoyproxy#14330) event: Reduce potential for lock contention while executing dispatcher post callbacks. (envoyproxy#14289) stale: fix config (envoyproxy#14337) metrics service sink: generalize the sink and grpc streamer for external use (envoyproxy#13919) wasm: update V8 to v8.8.278.8. (envoyproxy#14298) repo: switch to actions based stale bot (envoyproxy#14335) buffer: Use WatermarkFactory to create most WatermarkBuffer instances (envoyproxy#14256) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
2 participants
Commit Message:
buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque
Additional Description:
Risk Level: medium. Subtle changes to low level data structures. Manual implementation of move constructors.
Testing: unit
Docs Changes: n/a
Release Notes: n/a