RFR 8203768 : Avoid reallocation in java.base/unix/classes/java/lang/ProcessImpl.java

[Ivan Gerasimov]

Hej Martin!


On 6/4/18 9:55 PM, Martin Buchholz wrote:
> Hej Ivan,
>
> IIRC I wrote the first iteration of this code.
>
> I would expect that almost always either the buffer will be empty or 
> the call to available() is accurate and the read will actually read 
> everything in one chunk, so the byte array will be perfectly allocated 
> with no need for re-allocation in practice.  Do you have evidence it 
> would be otherwise in practice?
>
No evidence.  It is just an observation from reading code.

> But suppose available() incorrectly claims that a million bytes are 
> available, but you only read 10.  Then there's a tradeoff - the cost 
> of reallocation versus the risk that the mostly empty backing array 
> will be retained for a long time if the Process object is not garbage 
> collected.
>
On the other hand, if available() claims a million bytes, and then only 
999999990 bytes were read, there will be mostly unnecessary allocation 
and copying.

> I'm leaning towards the status quo.
>
Okay, let's leave it as is :)
It was meant mostly for cleaning up the code, so if there is a doubt, 
then it's better keep what we have.

With kind regards,
Ivan

>
> On Mon, Jun 4, 2018 at 9:26 PM, Ivan Gerasimov 
> <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote:
>
>     Hello!
>
>     When closing a Process, its stdout and stderr are drained: The
>     remaining bytes are copied into an array, and then the out/err
>     stream is replaced with ByteArrayInputStream().
>
>     In a case when a fewer number of bytes were read then the
>     Stream.available() suggested, the array is reallocated with
>     Arrays.copyOf().
>
>     It is possible to avoid reallocation, and use three-args
>     ByteArrayInputStream() constructor to specify a portion of the
>     array used.
>
>     Would you please help review this trivial fix?
>
>     BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203768
>     <https://bugs.openjdk.java.net/browse/JDK-8203768>
>     WEBREV: http://cr.openjdk.java.net/~igerasim/8203768/00/webrev/
>     <http://cr.openjdk.java.net/%7Eigerasim/8203768/00/webrev/>
>
>     All existing tests pass on all supported platforms.
>
>     Thanks in advance!
>
>     -- 
>     With kind regards,
>     Ivan Gerasimov
>
>

-- 
With kind regards,
Ivan Gerasimov