Final call Re: RFR: JDK-8230744 Several classes throw OutOfMemoryError without message

Wed Jun 10 22:02:32 UTC 2020

Regarding the wording, I don't think either "implementation limit" or "VM limit" 
is correct, at least not in all the cases.

If we were using ArraysSupport.newLength, the error messages are as follows:

     1) Actual shortage of heap space. Hotspot generates this:

     OutOfMemoryError: Java heap space

     2) Requested array size exceeds implementation limit. Hotspot
     generates this:

     OutOfMemoryError: Requested array size exceeds VM limit

     3) Integer wraparound/overflow during size computation.
     ArraysSupport.newLength() generates this:

     OutOfMemoryError: Required array length too large

For this changeset, we're leaving several places as they are, either using the 
*exact arithmetic methods or doing their own checks. OK, we'll save refactoring 
to ArraysSupport.newLength for another time. But the resulting OOME message is I 
think misleading.

For example, in String.java line 2190, we catch an exception from Math.addExact 
or Math.multiplyExact. This occurs if the arithmetic result exceeds the range of 
an int. It's less about some implementation or VM limit, but instead the 
requested length simply cannot be represented by an int.

Also, while I'm not as allergic to talking about arrays as Martin, in this case 
the subsequent code allocates a StringBuilder and not an array. So it is 
somewhat odd for this message to refer to an array.

Anyway, instead of

     Requested array size exceeds VM limit

I'd suggest this:

     Requested size too large

(Or "length" instead of "size". "Length" is more consistent for arrays and 
strings, but oddly enough, Hotspot uses "array size".)

I think this wording could apply to String.java, StringLatin1.java,
StringUTF16.java, Pattern.java, UnsyncByteArrayOutputStream.java, and 
ByteArrayChannel.java. I guess we could try to provide a customized message for 
each area, but that wouldn't seem to me to add much value.

I'm not entirely sure what's going on in AbstractStringBuilder.java. It uses 
ArraysSupport.newLength(), which should either return an in-range value or 
throw. I don't understand why the code in ASB.newCapacity() is checking length 
against Integer.MAX_VALUE and throwing again. This shouldn't happen by the spec 
of AS.newLength(). I think that check (lines 258-259) can simply be removed.

s'marks

On 6/10/20 10:22 AM, Roger Riggs wrote:
> Hi Jim,
> 
> In Unsafe.java: 632: the "Unable to allocate 98494837395: doesn't read very well.
> Please add " bytes" to the end.
> 
> I would probably drop "array" from all the messages but at least in the String* 
> cases.
> As Martin notes, the array is an implementation detail.
> (And i still prefer "implementation limit" over "VM limit".
> 
> Thanks, Roger
> 
> 
> 
> On 6/10/20 12:33 PM, Jim Laskey wrote:
>>
>>> On Jun 130, 2020, at 1:15 PM, Martin Buchholz <martinrb at google.com> wrote:
>>>
>>> I took a look at PriorityBlockingQueue.
>>>
>>> Part of converting to ArraysSupport involves deleting the local orphan
>>> MAX_ARRAY_SIZE; that needs to be done.
>> Removed.
>>
>>> ---
>>> It looks like
>>> newCap > oldCap
>>> is always true, so we should delete the test?
>>>                  if (newCap > oldCap && queue == array)
>>> ---
>> If oldCap == MAX_ARRAY_SIZE wouldn't newCap == oldCap
>>
>>
>>> In Pattern.java I see
>>> +            throw new OutOfMemoryError("Requested array size exceeds
>>> VM limit");
>>>
>>> That wording doesn't seem useful to me - the use of an array is an
>>> implementation detail, and the user didn't __request__ it.
>>>
>>> Better seems the wording in ArraysSupport
>>>             throw new OutOfMemoryError("Required array length too large");
>>> but if we're going to the trouble of composing a custom detail
>>> message, I'd try harder to find one meaningful to the user of the API,
>>> something like "pattern too large"
>> Done and done.
>>
>> Thank you.
>>
>>
>>
>>> On Wed, Jun 10, 2020 at 5:15 AM Jim Laskey <james.laskey at oracle.com> wrote:
>>>> Will push if no comments by EOB.
>>>>
>>>>> On Jun 8, 2020, at 2:22 PM, Jim Laskey <james.laskey at oracle.com> wrote:
>>>>>
>>>>> Revised to use a consistent error message. Modified AbstractStringBuilder 
>>>>> and PriorityBlockingQueue to use ArraysSupport.newLength(). Didn't modify 
>>>>> ByteArrayChannel and UnsyncByteArrayOutputStream since those changes would 
>>>>> require changes to module exporting.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~jlaskey/8230744/webrev-03/index.html 
>>>>> <http://cr.openjdk.java.net/~jlaskey/8230744/webrev-03/index.html>
>>>>>
>>>>>
>>>>>> On Jun 3, 2020, at 11:24 AM, Jim Laskey <james.laskey at oracle.com> wrote:
>>>>>>
>>>>>> It's not the goal or role of this bug to fix the wrongs of the past, 
>>>>>> merely add error messages to the exceptions. I raised the discussion as an 
>>>>>> issue. Clearly there is a correct path to follow. If you think more effort 
>>>>>> is required then file a bug. :-)
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> -- Jim
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Jun 2, 2020, at 7:13 PM, Stuart Marks <stuart.marks at oracle.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/2/20 6:52 AM, Jim Laskey wrote:
>>>>>>>> Revised to reflect requested changes.
>>>>>>>> http://cr.openjdk.java.net/~jlaskey/8230744/webrev-01/index.html 
>>>>>>>> <http://cr.openjdk.java.net/~jlaskey/8230744/webrev-01/index.html>
>>>>>>> On this, if all you're doing is changing exception messages, then I don't 
>>>>>>> care very much, modulo concerns about wording from others. If you start 
>>>>>>> to get into changing the growth logic (like the Math.addExact stuff) then 
>>>>>>> please see my message on the related thread, "Sometimes constraints are 
>>>>>>> questionable."
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> s'marks
> 