Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Martin Buchholz]

I decided to care just enough about the last 2x of scalability, but not
about the last 8 elements.
Your code would resize the 2g elements 8 times before finally reaching
MAX_VALUE...

Now I'm back in not-caring-anymore mode.
At least about MAX_ARRAY_SIZE.


On Wed, Aug 27, 2014 at 3:41 AM, Ulf Zibis  wrote:

> Am 25.08.2014 um 19:37 schrieb Martin Buchholz:
>
>  https://bugs.openjdk.java.net/browse/JDK-8055949
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/
>> ByteArrayOutputStream-MAX_ARRAY_SIZE/
>>
>> The 2x capacity gap was noticed by real users!
>>
>
> Hi Martin,
>
> the MAX_ARRAY_SIZE code now is copied to many places in JDK. Couldn't you
> better centralize the code to one place, e.g. j.u.Arrays or some hidden
> class of sun.java...?
> Isn't there a property to retrieve MAX_ARRAY_SIZE from the running VM?
>
> Imagine, some VM throws OOME above Integer.MAX_VALUE-4 and minCapacity is
> Integer.MAX_VALUE-4.
> With this code a OOME will happen:
>  124 return (minCapacity > MAX_ARRAY_SIZE) ?
>  125 Integer.MAX_VALUE :
>  126 MAX_ARRAY_SIZE;
> With this code we would avoid the OOME:
>  124 return (minCapacity > MAX_ARRAY_SIZE) ?
>  125 minCapacity :
>  126 MAX_ARRAY_SIZE;
>
> -Ulf
>
>

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Ulf Zibis]

Am 25.08.2014 um 19:37 schrieb Martin Buchholz:

https://bugs.openjdk.java.net/browse/JDK-8055949
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ByteArrayOutputStream-MAX_ARRAY_SIZE/

The 2x capacity gap was noticed by real users!


Hi Martin,

the MAX_ARRAY_SIZE code now is copied to many places in JDK. Couldn't you better centralize the code 
to one place, e.g. j.u.Arrays or some hidden class of sun.java...?

Isn't there a property to retrieve MAX_ARRAY_SIZE from the running VM?

Imagine, some VM throws OOME above Integer.MAX_VALUE-4 and minCapacity is 
Integer.MAX_VALUE-4.
With this code a OOME will happen:
 124 return (minCapacity > MAX_ARRAY_SIZE) ?
 125 Integer.MAX_VALUE :
 126 MAX_ARRAY_SIZE;
With this code we would avoid the OOME:
 124 return (minCapacity > MAX_ARRAY_SIZE) ?
 125 minCapacity :
 126 MAX_ARRAY_SIZE;

-Ulf

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Martin Buchholz]

I'll submit soonish.

@summary second line indented.

I added some gratuitous assertions to the test:

// check data integrity while we're here
byte[] bytes = baos.toByteArray();
if (bytes.length != n)
throw new AssertionError("wrong length");
if (bytes[0] != 'x' ||
bytes[bytes.length - 1] != 'x')
throw new AssertionError("wrong contents");



On Tue, Aug 26, 2014 at 12:07 AM, Alan Bateman 
wrote:

>
> In the mean-time then my only concern is that pushing a test with @ignore
> is just adding to the technical debt. Such tests are unlikely to be run or
> maintained. If you do push it with @test then a minor comment is to maybe
> push in the second line of @summary so that it is more distinguishable from
> the @ lines.


I don't think it particularly adds to the technical debt because there's an
existing pile of @ignored tests that share the same reason, that will all
be handled together ... in the fullness of time.

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Alan Bateman]

On 25/08/2014 21:45, Martin Buchholz wrote:

:

jtreg tests are run by default with -ignore:quiet and there is 
precedent for other tests with such @ignore statements, and I actually 
used jtreg -ignore:run to really test this.


What I would really like someday is to be able to run expensive tests 
(like this one) only if a user expressly requested them.


Anyways, I'm intending to submit as is unless you feel strongly.
The issue here is that we still lack a means to select tests based on 
resources and other criteria. I hope this come in time so it will be 
possible to write tests like this properly.


In the mean-time then my only concern is that pushing a test with 
@ignore is just adding to the technical debt. Such tests are unlikely to 
be run or maintained. If you do push it with @test then a minor comment 
is to maybe push in the second line of @summary so that it is more 
distinguishable from the @ lines.


-Alan.

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Mike Duigou]

This looks fine to me as well.

I am fine with the @ignore as I don't suspect anyone would be able to sneak in 
a change which removed the @ignore without anyone noticing and the comment for 
why it is marked @ignore seems adequate.

Mike

On Aug 25 2014, at 13:28 , Alan Bateman  wrote:

> On 25/08/2014 18:37, Martin Buchholz wrote:
>> Hi friends of ByteArrayOutputStream,
>> 
>> I'm trying to clean up an apparent oversight when I tried to fix huge array 
>> resizing back in
>> 6933217: Huge arrays handled poorly in core libraries
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8055949
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ByteArrayOutputStream-MAX_ARRAY_SIZE/
>>  
>> 
>> 
>> The 2x capacity gap was noticed by real users!
> The change to BAOS looks okay, I don't recall why it wasn't updated with the 
> previous changes.
> 
> I'm not sure about adding a test with @ignore though, maybe the @test should 
> just be dropped to avoid the temptation to "fix" the test before there is 
> support for selecting tests based on the resources available.
> 
> -Alan

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Martin Buchholz]

Thanks, Alan.


On Mon, Aug 25, 2014 at 1:28 PM, Alan Bateman 
wrote:

>  On 25/08/2014 18:37, Martin Buchholz wrote:
>
>  Hi friends of ByteArrayOutputStream,
>
>  I'm trying to clean up an apparent oversight when I tried to fix huge
> array resizing back in
> 6933217: Huge arrays handled poorly in core libraries
>
>  https://bugs.openjdk.java.net/browse/JDK-8055949
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ByteArrayOutputStream-MAX_ARRAY_SIZE/
>
>  The 2x capacity gap was noticed by real users!
>
> The change to BAOS looks okay, I don't recall why it wasn't updated with
> the previous changes.
>
> I'm not sure about adding a test with @ignore though, maybe the @test
> should just be dropped to avoid the temptation to "fix" the test before
> there is support for selecting tests based on the resources available.
>
>
jtreg tests are run by default with -ignore:quiet and there is precedent
for other tests with such @ignore statements, and I actually used jtreg
-ignore:run to really test this.

What I would really like someday is to be able to run expensive tests (like
this one) only if a user expressly requested them.

Anyways, I'm intending to submit as is unless you feel strongly.

Re: RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Alan Bateman]

On 25/08/2014 18:37, Martin Buchholz wrote:

Hi friends of ByteArrayOutputStream,

I'm trying to clean up an apparent oversight when I tried to fix huge 
array resizing back in

6933217: Huge arrays handled poorly in core libraries

https://bugs.openjdk.java.net/browse/JDK-8055949
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ByteArrayOutputStream-MAX_ARRAY_SIZE/ 



The 2x capacity gap was noticed by real users!
The change to BAOS looks okay, I don't recall why it wasn't updated with 
the previous changes.


I'm not sure about adding a test with @ignore though, maybe the @test 
should just be dropped to avoid the temptation to "fix" the test before 
there is support for selecting tests based on the resources available.


-Alan

RFR: 8055949: ByteArrayOutputStream capacity should be maximal array size permitted by VM

[Martin Buchholz]

Hi friends of ByteArrayOutputStream,

I'm trying to clean up an apparent oversight when I tried to fix huge array
resizing back in
6933217: Huge arrays handled poorly in core libraries

https://bugs.openjdk.java.net/browse/JDK-8055949
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ByteArrayOutputStream-MAX_ARRAY_SIZE/

The 2x capacity gap was noticed by real users!