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.mcma...@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.mcma...@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 >> >> >>
