RFR 8184285: Buffer sizes of Flow based BodyProcessor API

[Chris Hegarty]

Tobias, Michael,

I did a few rounds of iteration on this, that address Tobias’s comments ( modulo the variable buffer sizing, which we agreed to defer for now unless there is a compelling use case ). With this change, there is zero-copy of data. The composite container, List, is relatively lightweight ( and we need some type for composition anyway ( no need to invent something new )).

http://cr.openjdk.java.net/~chegar/httpBuffering/

I pushed this version to the sandbox, where can get further experience with it [1].

-Chris.

[1] http://hg.openjdk.java.net/jdk10/sandbox/jdk/rev/c243b0e0f9a6

hg clone http://hg.openjdk.java.net/jdk10/sandbox
cd sandbox
bash get_source.sh
bash common/bin/hgforest.sh -s update http-client-branch
# configure, make, etc


> On 4 Aug 2017, at 15:05, Michael McMahon <michael.x.mcmahon at oracle.com> wrote:
> 
> Hi Tobias,
> 
> On 04/08/2017, 10:22, Tobias Thierer wrote:
>> Hi Michael -
>> 
>> thanks for your work! The fact that BufferingProcessor wraps another BodyProcessor looks like it could be quite useful/flexible.
>> 
>> Some things I noticed upon a quick glance:
>> 	• BufferingProcessor's documentation cuts out mid-sentence.
> That could be just how the unformatted text looks. I've uploaded the HTML apidoc
> for the two interfaces to:
> http://cr.openjdk.java.net/~michaelm/8184285/apidoc/
> which should be easier to read. Links won't work but you can just search for the buffering() method in both docs.
>> 	• BufferingProcessor passes modifiable LinkedList instances. Why? Do you want to guarantee that remove-from-front is efficient? If not, consider singleton and array-based unmodifiable instances since they tend to be faster, more memory efficient, and don't risk applications starting to accidentally rely on the lists being modifiable? 
> That seems like a good idea.
>> 	• Unsurprisingly, getBuffersOf() performs a copy via getNBytesFrom() - just a thing to be aware of, but I don't expect it can be avoided.
>> 	• The buffer size is fixed, so one can't change the buffer size dynamically (e.g. in response to bitrate changes in a video player app) or even make the first buffer a different size than the later ones (e.g. file header vs. later chunks). I don't know a good solution to the latter either, since Flow.request(long) documents that the long refers to the number of calls; the former looks like it could be made more flexible in a future version of BufferingProcessor, but is probably okay for now.
> At the very least it would require defining a new subtype of HttpResponse.BufferingProcessor and it complicates the Flow semantics
> a bit, because you could be changing the buffersize while there is outstanding demand at the old buffersize. But, it could be usable
> if you stick to doing request(1) all the time. I think we should defer on that for now though.
>> 	• So far it looks like the new code will interoperate with blocking facades such as PipedResponseStream, which is nice. I hope to verify this empirically next week.
>> I intend to write more comments later, just wanted to send a quick first impression. Thanks for the promising update!
>> 
> Great thanks for the first impression.
> 
> Michael
>> Tobias
>> 
>> On Thu, Aug 3, 2017 at 10:02 AM, Michael McMahon <michael.x.mcmahon at oracle.com> wrote:
>> Hi,
>> 
>> The HTTP client work is continuing in a new branch of the JDK 10 sandbox forest (http-client-branch),
>> and here is the first of a number of changes we want to make.
>> 
>> This one is to address the feedback we received where HttpResponse.BodyProcessors would
>> be easier to implement if there was control over the size of buffers being supplied.
>> 
>> To that end we have added APIs for creating buffered response processors (and handlers)
>> 
>> So, HttpResponse.BodyProcessor has a new static method with the following signature
>> 
>> public static <T> BodyProcessor<T> buffering(BodyProcessor<T> downstream, long buffersize) {}
>> 
>> This returns a new processor which delivers data to the supplied downstream processor, buffered
>> by the 'buffersize' parameter. It guarantees that all data is delivered in chunks of that size
>> until the final chunk, which may be smaller.
>> 
>> This should allow other BodyProcessor implementations that require buffering to wrap themselves
>> in this way, be guaranteed that the data they receive is buffered, and then return that composite
>> processor to their user.
>> 
>> A similar method is added to HttpResponse.BodyHandler.
>> 
>> Note also, that we have changed HttpResponse.BodyProcessor from being a Flow.Subscriber<ByteBuffer>
>> to Flow.Subscriber(List<ByteBuffer>). That change is technically orthogonal to this one, but is motivated
>> by it. By transferring ByteBuffers in lists makes it easier to buffer them efficiently.
>> 
>> The webrev is at: http://cr.openjdk.java.net/~michaelm/8184285/webrev.1/
>> 
>> Thanks,
>> Michael
>> 
>> 
>>