Improvements to grpc_byte_stream API and handling. #11905

markdroth · 2017-07-21T17:44:41Z

Add shutdown() method (to be used in forthcoming call combiner code).
Use a vtable instead of storing method pointers in each instance.
Check all callers of pull() to make sure that they are properly
handling errors.
Clarify ownership rules and attempt to adhere to them.
Added a new grpc_caching_byte_stream implementation, which is used in
http_client_filter to avoid having to read the whole send_message byte
stream before passing control down the stack. (This class may also be
used in the retry code I'm working on separately.)
As part of this, did a major rewrite of http_client_filter, which
made the code more readable and fixed a number of potential bugs.

Note that some of this code is hard to test right now, due to the fact
that the send_message byte stream is always a slice_buffer stream, for
which next() is always synchronous and no destruction is needed.
However, some future work (specifically, my call combiner work and
Craig's incremental send work) will start leveraging this.

@vjpai Please look at the inproc_transport code.
@ctiller Please carefully scrutinize the chttp2 code.

@makdharma FYI, you may want to look at the changes in http_client_filter.

grpc-kokoro · 2017-07-21T18:56:46Z

[trickle] No significant performance differences

grpc-kokoro · 2017-07-21T19:14:19Z

[microbenchmarks] No significant performance differences

vjpai · 2017-07-21T19:18:12Z

src/core/ext/transport/inproc/inproc_transport.c

-  s->base.next = inproc_slice_byte_stream_next;
-  s->base.pull = inproc_slice_byte_stream_pull;
-  s->base.destroy = inproc_slice_byte_stream_destroy;
+  s->base.vtable = &inproc_slice_byte_stream_vtable;


This saves space (1 pointer instead of 3) but increases the dependence length. It was load->jump but is now load->load->jump (this applies to all of the byte_stream classes that use the vtable). Are there really enough active instances that the memory savings is important?

I haven't taken any measurements or anything, but I'd say yes: there will be at least one grpc_byte_stream instance for each sent and received message. In fact, sometimes there will be more than one byte stream for a given sent or received message -- for example, in a case like the new grpc_cached_byte_stream class, where one byte stream wraps another one.

Also, note that with the addition of the new shutdown method, it's actually 4-to-1, not 3-to-1.

If there's compelling evidence that this is going to be a performance problem, we can certainly revert it. But I think it actually makes the code cleaner.

There's also a consistency argument: most all types we have have this vtable configuration.

ncteisen

Didn't comment on the changes to the filters bc I am not as familiar with those paths. I will take a look next week, but someone else should do a deeper review of them.

ncteisen · 2017-07-21T22:39:34Z

src/core/ext/transport/chttp2/transport/chttp2_transport.c

@@ -2768,6 +2760,33 @@ grpc_error *grpc_chttp2_incoming_byte_stream_finished(
  return error;
 }

+static void incoming_byte_stream_shutdown(grpc_exec_ctx *exec_ctx,


Would shutdown ever get called with GRPC_ERROR_NONE? In that case isn't there a chance that we would lose some error fidelity. If bs->remaining_bytes > 0, then grpc_chttp2_incoming_byte_stream_finished would create an error, but it would get lost and never seen, because it would be unreffed here.

Relatedly, doesn't this fail the invariant that the error (whatever it may be) passed to shutdown will be returned to all future calls to pull? Because this shutdown, resets the stream, which will clear the error.

No, I think shutdown should never be called with GRPC_ERROR_NONE.

I didn't intend the invariant here to be that all subsequent calls to pull() return the error, only that the next call to pull() returns the error. In practice, once a caller gets an error from pull(), they're not going to call it again. I've attempted to clarify the wording in byte_stream.h about this.

ncteisen · 2017-07-21T23:09:20Z

src/core/ext/transport/inproc/inproc_transport.c

@@ -72,6 +72,7 @@ typedef struct sb_list_entry {
 typedef struct {
  grpc_byte_stream base;
  sb_list_entry *le;
+  grpc_error *shutdown_error;


Maybe it makes sense to pull this into the base struct? I know grpc_chttp2_incoming_byte_stream doesn't have it at the moment, but maybe it should based on my other comments

I thought about this, but I suspect that there will be implementations (such as the one in chttp2) that will want to handle shutdown differently.

ncteisen · 2017-07-21T23:15:40Z

src/core/lib/transport/byte_stream.c

+  grpc_error *error =
+      grpc_byte_stream_pull(exec_ctx, stream->cache->underlying_stream, slice);
+  if (error == GRPC_ERROR_NONE) {
+    grpc_slice_buffer_add(&stream->cache->cache_buffer,


I am a little confused by this. We are caching it in the cache buffer, but also passing it pack up to the original caller? In what situations would the cache grow large enough that calls to this pull would only draw from the cache and not the underlying byte stream?

Ahh, I think I understand now after reading the comments in the header. IS this all for cases in which someone has reset the buffer?

Yes, resetting the caching byte stream is one case. The other case is where you have multiple grpc_caching_byte_stream objects using the same grpc_byte_stream_cache, which is something I'll be doing in the retry code.

ncteisen · 2017-07-21T23:21:31Z

src/core/lib/transport/byte_stream.h

+  grpc_byte_stream_cache *cache;
+  size_t cursor;
+  grpc_error *shutdown_error;
+} grpc_caching_byte_stream;


I see this isn't used anywhere yet. We should add a unit test for it (and maybe the other byte streams if possible) before it actually gets used for future projects.

It's used in http_client_filter. But you're right, a test would be good.

Done (and fixed a bug I found in the process, thus confirming that a test was a good idea).

markdroth · 2017-07-24T14:57:42Z

@dgquintas, can you please look at the filter changes? Thanks!

grpc-kokoro · 2017-07-24T16:42:39Z

[trickle] No significant performance differences

ncteisen

Small nit. Other than that, LGTM. (But still best to have someone with more filter experience do a pass over those files)

ncteisen · 2017-07-24T16:32:37Z

src/core/ext/filters/http/client/http_client_filter.c

+  grpc_call_next_op(exec_ctx, elem, calld->send_message_batch);
+}
+
+static char *slice_buffer_to_string(grpc_slice_buffer *slice_buffer) {


This seems like a function that belongs in a util file

I'm not sure if this is something that is needed in any other context. Let's keep it here for now, but we can move it if we find ourselves needing it in a different place.

grpc-kokoro · 2017-07-24T17:14:56Z

[microbenchmarks] Performance differences noted:
Benchmark                                  cpu_time    real_time
-----------------------------------------  ----------  -----------
BM_IsolatedFilter<HttpClientFilter, NoOp>  -4%         -4%

dgquintas

LGTM for filters

vjpai · 2017-07-26T20:16:14Z

LGTM

markdroth · 2017-07-26T21:29:13Z

Known failures: #11911 #11055 #10807 #11745

- Add shutdown() method (to be used in forthcoming call combiner code). - Use a vtable instead of storing method pointers in each instance. - Check all callers of pull() to make sure that they are properly handling errors. - Clarify ownership rules and attempt to adhere to them. - Added a new grpc_caching_byte_stream implementation, which is used in http_client_filter to avoid having to read the whole send_message byte stream before passing control down the stack. (This class may also be used in the retry code I'm working on separately.) - As part of this, did a major rewrite of http_client_filter, which made the code more readable and fixed a number of potential bugs. Note that some of this code is hard to test right now, due to the fact that the send_message byte stream is always a slice_buffer stream, for which next() is always synchronous and no destruction is needed. However, some future work (specifically, my call combiner work and Craig's incremental send work) will start leveraging this.

markdroth assigned ncteisen, vjpai and ctiller Jul 21, 2017

markdroth requested review from ncteisen, vjpai and ctiller July 21, 2017 17:44

googlebot added the cla: yes label Jul 21, 2017

vjpai reviewed Jul 21, 2017

View reviewed changes

ncteisen suggested changes Jul 21, 2017

View reviewed changes

markdroth requested a review from dgquintas July 24, 2017 14:57

markdroth assigned dgquintas Jul 24, 2017

ncteisen approved these changes Jul 24, 2017

View reviewed changes

dgquintas approved these changes Jul 25, 2017

View reviewed changes

markdroth force-pushed the byte_stream_shutdown branch from 8aafcfe to 5794061 Compare July 26, 2017 21:30

markdroth merged commit c44f036 into grpc:master Jul 26, 2017

markdroth deleted the byte_stream_shutdown branch July 26, 2017 21:30

markdroth mentioned this pull request Jul 27, 2017

Implement tee_byte_stream. #10448

Closed

markdroth mentioned this pull request Feb 13, 2018

Take ownership of byte_buffer contents as soon as send_message op is started. #14416

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

lock bot unassigned dgquintas Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to grpc_byte_stream API and handling. #11905

Improvements to grpc_byte_stream API and handling. #11905

markdroth commented Jul 21, 2017

grpc-kokoro commented Jul 21, 2017

grpc-kokoro commented Jul 21, 2017

vjpai Jul 21, 2017

markdroth Jul 21, 2017

ctiller Jul 24, 2017

ncteisen left a comment

ncteisen Jul 21, 2017

ncteisen Jul 21, 2017

markdroth Jul 24, 2017

ncteisen Jul 21, 2017

markdroth Jul 24, 2017

ncteisen Jul 21, 2017

ncteisen Jul 21, 2017

markdroth Jul 24, 2017

ncteisen Jul 21, 2017

markdroth Jul 24, 2017

markdroth commented Jul 24, 2017

grpc-kokoro commented Jul 24, 2017

ncteisen left a comment

ncteisen Jul 24, 2017

markdroth Jul 25, 2017

grpc-kokoro commented Jul 24, 2017

dgquintas left a comment

vjpai commented Jul 26, 2017

markdroth commented Jul 26, 2017

Improvements to grpc_byte_stream API and handling. #11905

Improvements to grpc_byte_stream API and handling. #11905

Conversation

markdroth commented Jul 21, 2017

grpc-kokoro commented Jul 21, 2017

grpc-kokoro commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncteisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth commented Jul 24, 2017

grpc-kokoro commented Jul 24, 2017

ncteisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grpc-kokoro commented Jul 24, 2017

dgquintas left a comment

Choose a reason for hiding this comment

vjpai commented Jul 26, 2017

markdroth commented Jul 26, 2017