buffer: add method to extract front slice without copying

[roelfdutoit]

Add a method to Envoy::Buffer::Instance that may be used to extract the front slice of the implementation's queue (SliceDeque in the case of Buffer::OwnedImpl) without copying the actual payload. A SliceData class is defined to facilitate the extraction process.

Risk Level: Low
Testing: Additional unit tests
Docs Changes: n/a
Release Notes: n/a
Fixes #12373

[antoniovicente]

Thanks for putting together a PR that satisfies your feature request. Some API questions and comments regarding interaction of the new method with existing buffer constructs.

[antoniovicente]

I think this should be something like absl::Span<const uint8_t>

I had not considered const issues in the existing buffer API which was a fairly direct translation of an earlier API built on top of libevent buffers. IIRC data in Unowned slices should be considered immutable. It is technically possible for multiple unowned slices to share the same underlying storage although I think we don't rely on that yet, but in-progress enhancements including the HTTP/1.1 cache extension would benefit from the ability to provide const access to data blocks that are referenced by buffers so multiple requests can reference the same data blocks in the in-memory cache.

Is use of const data blocks compatible with your use case?

[roelfdutoit]

My use case requires a mutable slice, but I agree that the more common case would be an immutable view of the slice. Commit df19673 attempts to provide both, with immutable as default.

[antoniovicente]

Calls to this method should crash.

[antoniovicente]

Also, move this implementation to UnownedSlice.

[roelfdutoit]

Crash as in ASSERT?

[roelfdutoit]

I had it in UnownedSlice, but decided to move to Slice because:

1. It is the default pre-override behavior.
2. DummySlice in buffer_test.cc would have to change.

[roelfdutoit]

Will use the following:

  absl::Span<uint8_t> getMutableData() override {
    RELEASE_ASSERT(isMutable(), "Not allowed to call getMutableData if slice is immutable");
    return {base_ + data_, reservable_ - data_};
  }

[roelfdutoit]

Changed in commit adde208

[antoniovicente]

I'm sorry for the back and forth about APIs, but I'ld like your opinion on the following:
Should we focus on the mutable slice case and avoid the non mutable case entirely. For OwnedSlices, the extraction method can just return the slice, but in the case of UnownedSlice it would force copy on extraction. Effectively, keep extractMutableFrontSlice and getMutableData methods from the APIs added in this PR, but remove extractFrontSlice, getData and isMutable.

For consistency between these two cases, I think that drain trackers should be cleared from OwnedSlices as part of the extraction process.

[antoniovicente]

Changes look good. Some minor test issues remain, the bigger one is related to testing of drain trackers on unowned slices.