Add TranscoderInputStreamImpl by lizan · Pull Request #1078 · envoyproxy/envoy

lizan · 2017-06-09T20:01:39Z

The class implements TranscoderInputStream from transcoding library,
take input from Envoy::Buffer, this also allows Envoy::Buffer to be used
as protobuf::io::ZeroCopyInputStream.

For #501

The class implements TranscoderInputStream from transcoding library, take input from Envoy::Buffer, this also allows Envoy::Buffer to be used as protobuf::io::ZeroCopyInputStream. For envoyproxy#501

lizan · 2017-06-09T20:04:42Z

#1079 for the context how this is used.

mattklein123 · 2017-06-09T21:47:46Z

@lizan Can we make a note to use this here as well: https://github.com/lyft/envoy/blob/master/source/common/grpc/rpc_channel_impl.cc#L62

Doesn't need to be done in this PR if you don't want, but would like to do a follow up. Will catch up on reviews this weekend.

htuch

I think this is useful, but do we want to couple zero-copy with the transcoder? As @mattklein123 points out, this is useful in other places as well (including the active #1054 under review). Can we have a protobuf zero-copy Buffer subclass (or build this into Buffer), then use this here in the TranscoderInputStreamImply?

htuch · 2017-06-12T21:25:14Z

source/common/grpc/transcoder_input_stream_impl.h

+
+#include "common/buffer/buffer_impl.h"
+
+#include "src/transcoder_input_stream.h"


Can you wire up the external deps for this PR? Ideally, the grpc_transcoder library doesn't live under the src prefix, but something more like grpc_transcoder/....

Also, do you want to add mocks for the TranscoderInputStream interface?

grpc-ecosystem/grpc-httpjson-transcoding#9 for prefix.

Probably not need mocks, just like buffers, it is hard to EXPECT how the input stream is used, so just using real impl should be fine.

htuch · 2017-06-12T21:27:09Z

source/common/grpc/transcoder_input_stream_impl.h

+  void Move(Buffer::Instance& instance);
+
+  // Mark the buffer is finished
+  void Finish() { finished_ = true; }


For all methods that are not inherited from external deps, please follow the Envoy convention and start methods with lower case as per the style guide.

htuch · 2017-06-12T21:31:36Z

source/common/grpc/transcoder_input_stream_impl.cc

+  }
+
+  Buffer::RawSlice slice;
+  uint64_t num_slices = buffer_.getRawSlices(&slice, 1);


Nit: const uint64_t num_slices = ...

htuch · 2017-06-12T21:31:46Z

source/common/grpc/transcoder_input_stream_impl.cc

+  Buffer::RawSlice slice;
+  uint64_t num_slices = buffer_.getRawSlices(&slice, 1);
+
+  if (num_slices) {


Nit: if (num_slices > 0) {

htuch · 2017-06-12T21:34:59Z

source/common/grpc/transcoder_input_stream_impl.cc

+bool TranscoderInputStreamImpl::Skip(int) { NOT_IMPLEMENTED; }
+
+void TranscoderInputStreamImpl::BackUp(int count) {
+  GOOGLE_CHECK_GE(count, 0);


What are these macros? They aren't standard in the Envoy code base. Could we just use an ASSERT?

htuch · 2017-06-12T21:36:22Z

source/common/grpc/transcoder_input_stream_impl.cc

+  GOOGLE_CHECK_LE(count, position_);
+
+  position_ -= count;
+  byte_count_ -= count;


Since I don't have the interface, I can't tell the semantics of byte_count_, but I noticed it isn't decremented if (position != 0), so in the absence of Backup, it monotonically increases forever?

https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.io.zero_copy_stream#ZeroCopyInputStream

byte_count_ here is for ByteCount(), which

Returns the total number of bytes read since this object was created.

So yes it monotonically increases forever.

…nput_stream

lizan · 2017-06-15T20:13:52Z

@htuch @mattklein123 refactored out ZeroCopyInputStreamImpl, and make AsyncClient and RpcChannel use it. PTAL.

mattklein123 · 2017-06-16T14:45:36Z

source/common/buffer/zero_copy_input_stream_impl.h

+
+class ZeroCopyInputStreamImpl : public virtual google::protobuf::io::ZeroCopyInputStream {
+public:
+  // Create input stream with one buffer, and finish immediately


I'm not sure if there is any easy way to do this, but in this case, it would be nice if we didn't have to allocate a new buffer on the stack, just to move into, and then operate. IMO we should be able to operate directly on the input buffer since it's non-const anyway. I would just put in a TODO here for a potential perf optimization.

Seems all usage are using InstancePtr, so just changed the signature with InstancePtr in this class.

mattklein123 · 2017-06-16T14:47:36Z

source/common/buffer/zero_copy_input_stream_impl.h

+  // Create input stream with empty buffer
+  ZeroCopyInputStreamImpl() {}
+
+  virtual ~ZeroCopyInputStreamImpl() {}


nit: is this needed to compile? otherwise del

mattklein123 · 2017-06-16T14:52:33Z

source/common/buffer/zero_copy_input_stream_impl.cc

+  }
+
+  Buffer::RawSlice slice;
+  const uint64_t num_slices = buffer_.getRawSlices(&slice, 1);


See comment here: https://github.com/lyft/envoy/blob/master/include/envoy/buffer/buffer.h#L67 I think this can return a slice that has no data in it. Just make sure the code below correctly handled that case. I think you probably need to also check that slice.len_ > 0 but not sure.

mattklein123 · 2017-06-16T14:54:27Z

source/common/buffer/zero_copy_input_stream_impl.cc

+
+bool ZeroCopyInputStreamImpl::Skip(int) { NOT_IMPLEMENTED; }
+
+void ZeroCopyInputStreamImpl::BackUp(int count) {


I'm a little hazy on the proto buffer interfaces, but is this always safe? We are draining() above. Can we BackUp() into nothing? Or does that never happen. If so I would add a comment.

Done.

The precondition of BackUp() doesn't allow them. so we're safe to draining. Added comments.

mattklein123 · 2017-06-16T14:57:56Z

source/common/buffer/zero_copy_input_stream_impl.cc

+    return true;
+  }
+
+  if (!finished_) {


I'm a little confused what happens here. If we return true here with no data, how does the proto code know to call Next() again without just going into a spin loop? Or is this only useful in the explicit transcoding case somehow? Again might need a few more comments for those not as familiar with proto interfaces.

It is callers responsibility to maintain. Added comment. transcoding interface add BytesAvailable() to avoid this.

…nput_stream

mattklein123

LGTM pending small grammar nits and any @htuch comments.

mattklein123 · 2017-06-20T15:02:33Z

source/common/buffer/zero_copy_input_stream_impl.cc

+  // Preconditions for BackUp:
+  // - The last method called must have been Next().
+  // - count must be less than or equal to the size of the last buffer returned by Next().
+  // Due to preconditions above, it is save to just adjust position_ and byte_count_ here, and


s/save/safe

mattklein123 · 2017-06-20T15:02:53Z

source/common/buffer/zero_copy_input_stream_impl.cc

+  // - The last method called must have been Next().
+  // - count must be less than or equal to the size of the last buffer returned by Next().
+  // Due to preconditions above, it is save to just adjust position_ and byte_count_ here, and
+  // draining in Next().


s/draining/drain

mattklein123 · 2017-06-20T15:03:13Z

source/common/buffer/zero_copy_input_stream_impl.h

  // google::protobuf::io::ZeroCopyInputStream
+  // See
+  // https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.io.zero_copy_stream#ZeroCopyInputStream
+  // for each methods details.


s/each methods/method

mattklein123 · 2017-06-20T15:04:45Z

source/common/buffer/zero_copy_input_stream_impl.h

+  // https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.io.zero_copy_stream#ZeroCopyInputStream
+  // for each methods details.
+
+  // Note the Next() will return true with no data until next data available if the stream is not


// Note Next() will return true with no data until more data is available if the stream is not
// finished. It is the caller's responsibility to finish the stream or wrap with LimitingInputStream
// before passing to protobuf code to avoid a spin loop.

htuch

LGTM, just some minor stuff.

htuch · 2017-06-20T19:21:29Z

source/common/buffer/zero_copy_input_stream_impl.h

+
+namespace Buffer {
+
+class ZeroCopyInputStreamImpl : public virtual google::protobuf::io::ZeroCopyInputStream {


Is virtual needed here? Is this because google::protobuf::io::ZeroCopyInputStream isn't pure? Do have potential diamond inheritance with this?

Yes, TranscoderInputStreamImpl inherits this class and TranscoderInputStream from transcoding library, which inherits ZeroCopyInputStream.

htuch · 2017-06-20T19:22:24Z

source/common/buffer/zero_copy_input_stream_impl.h

+
+private:
+  bool finished_{false};
+  int64_t byte_count_{0};


This can be negative?

changed to uint

htuch · 2017-06-20T19:25:19Z

test/common/buffer/zero_copy_input_stream_test.cc

+  ZeroCopyInputStreamImpl stream_;
+
+  const void* data_;
+  int size_;


Nit: Prefer fixed size types in general in Envoy .

data_ and size_ are used to receive output from Next, so types here matches the interface.

The most recent Envoy bump requires this: envoyproxy/envoy-mobile#1076 ``` The new binary is 2.00 % different in size compared to main. The new binary is 5790759 bytes. The current size (5790759) is larger than the maximum size (5700000). ``` Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This removes the unnecessary replace statement in go.mod. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Add TranscoderInputStreamImpl

6572487

The class implements TranscoderInputStream from transcoding library, take input from Envoy::Buffer, this also allows Envoy::Buffer to be used as protobuf::io::ZeroCopyInputStream. For envoyproxy#501

htuch reviewed Jun 12, 2017

View reviewed changes

lizan added 6 commits June 14, 2017 14:51

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

a8f8f33

…nput_stream

Refactor ZeroCopyInputStreamImpl out

579d342

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

118d1e7

…nput_stream

fix

ca46f94

Use ZeroCopyInputStreamImpl in AsyncClientImpl and RpcChannelImpl

8034159

fix

77f9c00

lizan added 2 commits June 15, 2017 15:35

fix format

dd68af1

fix finish test

8d235bd

mattklein123 reviewed Jun 16, 2017

View reviewed changes

lizan added 5 commits June 19, 2017 16:00

Address review comments

98c5ccf

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

547b92f

…nput_stream

fix format

8f1f307

fix test include

b88b0d5

fix format

9850c9b

mattklein123 reviewed Jun 20, 2017

View reviewed changes

htuch reviewed Jun 20, 2017

View reviewed changes

fix grammer in comments

53fcf47

mattklein123 previously approved these changes Jun 20, 2017

View reviewed changes

fix format

6be5d0d

lizan dismissed mattklein123’s stale review via 6be5d0d June 20, 2017 22:27

mattklein123 approved these changes Jun 20, 2017

View reviewed changes

mattklein123 merged commit f565599 into envoyproxy:master Jun 21, 2017

mathetake added a commit that referenced this pull request Mar 3, 2026

deps: removes unnecessary replace (#1078)

2a291cc

**Description** This removes the unnecessary replace statement in go.mod. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>


		#include "common/buffer/buffer_impl.h"

		#include "src/transcoder_input_stream.h"


		bool ZeroCopyInputStreamImpl::Skip(int) { NOT_IMPLEMENTED; }

		void ZeroCopyInputStreamImpl::BackUp(int count) {


		namespace Buffer {

		class ZeroCopyInputStreamImpl : public virtual google::protobuf::io::ZeroCopyInputStream {

Conversation

lizan commented Jun 9, 2017

Uh oh!

lizan commented Jun 9, 2017

Uh oh!

mattklein123 commented Jun 9, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!