8247373: ArraysSupport.newLength doc, test, and exception message #1617

stuart-marks · 2020-12-04T06:50:14Z

This rewrites the doc of ArraysSupport.newLength, adds detail to the exception message, and adds a test. In addition to some renaming and a bit of refactoring of the actual code, I also made two changes of substance to the code:

I fixed a problem with overflow checking. In the original code, if oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this method could return a negative value. It turns out that writing tests helps find bugs!
Under the old policy, if oldLength and minGrowth required a length above SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would return Integer.MAX_VALUE. That doesn't make any sense, because attempting to allocate an array of that length will almost certainly cause the Hotspot to throw OOME because its implementation limit was exceeded. Instead, if the required length is in this range, this method returns that required length.

Separately, I'll work on retrofitting various call sites around the JDK to use this method.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8247373: ArraysSupport.newLength doc, test, and exception message

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to 8892eb4
Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to bacb5f9
Martin Buchholz (@Martin-Buchholz - Reviewer) ⚠️ Review applies to bacb5f9
Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 8892eb4

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617
$ git checkout pull/1617

bridgekeeper · 2020-12-04T06:50:19Z

👋 Welcome back smarks! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-12-04T06:50:56Z

@stuart-marks The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2020-12-04T06:57:06Z

Webrevs

mlbridge · 2020-12-04T06:59:59Z

Mailing list message from Stuart Marks on core-libs-dev:

Hi Martin,

I'd appreciate it if you could take a look at this.

Thanks,

s'marks

On 12/3/20 10:57 PM, Stuart Marks wrote:

RogerRiggs

Nice clean description of the algorithm.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java

RogerRiggs · 2020-12-04T15:57:35Z

test/jdk/jdk/internal/util/ArraysSupport/NewLength.java

+            { SOFT+1, 1,      IMAX,   SOFT+2 },
+            { IMAX-2, 1,      IMAX,   IMAX-1 },
+            { IMAX-1, 1,      IMAX,   IMAX   }
+        };


Adding test cases for zero (0) for the min and preferred would be good to include and show any unpredictable behavior.

No, I don't want to add test cases that violate the preconditions. I guess I could add a prefGrowth==0 case though.

stefan-zobel · 2020-12-04T18:14:06Z

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java

+     * might exceed the JVM's implementation limit. In that case, the caller will likely
+     * attempt an array allocation with that length and encounter an OutOfMemoryError.
+     * Of course, regardless of the length value returned from this method, the caller
+     * may encounter OutOfMemoryError if there is insufficient heap to fufill the request.


typo in line 626: fufill -> fulfill

stuart-marks · 2020-12-05T03:01:58Z

For what it's worth, the following code:

var list = Collections.nCopies(Integer.MAX_VALUE - 3, "");
var list2 = new ArrayList<>(list);
list2.addAll(list);

results in java.lang.NegativeArraySizeException: -8 where the -8 is returned by ArraysSupport.newLength. This is a demonstration of the problem with overflow checking that is fixed by this change. With the fix, the result is

OutOfMemoryError: Required array length 2147483644 + 2147483644 is too large

openjdk · 2020-12-08T15:17:39Z

@stuart-marks This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8247373: ArraysSupport.newLength doc, test, and exception message

Reviewed-by: rriggs, psandoz, martin, prappo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1192 new commits pushed to the master branch:

96c4321: 8262424: Change multiple get_java_xxx() functions in thread.cpp into one function
0de6abd: 8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
85a5ae8: 8261606: Surprising behavior of step over in String switch
be67aaa: 8262726: AArch64: C1 StubAssembler::call_RT can corrupt stack
0f6122b: 8262819: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails with release VMs
dd33a8e: 8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
3b350ad: 8261710: SA DSO objects have sizes that are too large
fdd1093: 8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base
... and 1182 more: https://git.openjdk.java.net/jdk/compare/692b273ec53f54a879a4bbaad6c2f5f1d5358a71...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java

stuart-marks · 2020-12-17T00:45:54Z

@Martin-Buchholz Any comments on this?

bridgekeeper · 2021-01-14T02:52:54Z

@stuart-marks This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Martin-Buchholz

Many years ago, when I wrote the unfactored MAX_ARRAY_LENGTH code, I considered refactoring it, but didn't follow through because various implementations had too many small differences . I'm glad you made it work.

I'm happy to see good tests added. Testability is a benefit of refactoring.

I'm happy to see the name change to SOFT_MAX_ARRAY_LENGTH - that's a better name.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java

Martin-Buchholz · 2021-01-24T20:13:07Z

test/jdk/jdk/internal/util/ArraysSupport/NewLength.java

+        } catch (OutOfMemoryError oome) {
+            // ok


Instead of an //ok comment, I like to give the exception a name that makes the intent clear.

} catch (IllegalArgumentException success) {}

Martin-Buchholz · 2021-01-24T20:16:44Z

The new docs and tests are superb.

bridgekeeper · 2021-02-21T20:53:50Z

@stuart-marks This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

pavelrappo

Overall this looks good.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java

test/jdk/jdk/internal/util/ArraysSupport/NewLength.java

stuart-marks · 2021-03-02T18:07:43Z

/integrate

openjdk · 2021-03-02T18:08:47Z

@stuart-marks Since your change was applied there have been 1192 commits pushed to the master branch:

96c4321: 8262424: Change multiple get_java_xxx() functions in thread.cpp into one function
0de6abd: 8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
85a5ae8: 8261606: Surprising behavior of step over in String switch
be67aaa: 8262726: AArch64: C1 StubAssembler::call_RT can corrupt stack
0f6122b: 8262819: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails with release VMs
dd33a8e: 8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
3b350ad: 8261710: SA DSO objects have sizes that are too large
fdd1093: 8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base
... and 1182 more: https://git.openjdk.java.net/jdk/compare/692b273ec53f54a879a4bbaad6c2f5f1d5358a71...master

Your commit was automatically rebased without conflicts.

Pushed as commit f18c019.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8247373: ArraysSupport.newLength doc, test, and exception message

03b7922

openjdk bot added the core-libs [email protected] label Dec 4, 2020

stuart-marks marked this pull request as ready for review December 4, 2020 06:53

openjdk bot added the rfr Pull request is ready for review label Dec 4, 2020

RogerRiggs reviewed Dec 4, 2020

View reviewed changes

stefan-zobel reviewed Dec 4, 2020

View reviewed changes

fix typo, clarify asserts disabled, test prefGrowth==0

bacb5f9

RogerRiggs approved these changes Dec 8, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 8, 2020

PaulSandoz approved these changes Dec 9, 2020

View reviewed changes

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Outdated Show resolved Hide resolved

stuart-marks mentioned this pull request Jan 4, 2021

Consolidate declarations of MAX_ARRAY_SIZE in one place #1506

Closed

3 tasks

Martin-Buchholz reviewed Jan 24, 2021

View reviewed changes

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Outdated Show resolved Hide resolved

Martin-Buchholz reviewed Jan 24, 2021

View reviewed changes

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Show resolved Hide resolved

Martin-Buchholz reviewed Jan 24, 2021

View reviewed changes

Martin-Buchholz approved these changes Jan 24, 2021

View reviewed changes

stuart-marks added 3 commits March 1, 2021 21:58

Update with recommendations from Martin Buchholz.

6710004

Add newline at end of file.

c520e8e

Update copyright year.

8892eb4

RogerRiggs approved these changes Mar 2, 2021

View reviewed changes

pavelrappo approved these changes Mar 2, 2021

View reviewed changes

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Outdated Show resolved Hide resolved

test/jdk/jdk/internal/util/ArraysSupport/NewLength.java Show resolved Hide resolved

Fix spelling of HotSpot

68318d4

openjdk bot closed this Mar 2, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 2, 2021

stuart-marks mentioned this pull request Dec 19, 2023

8321637: Simplify if statement in ArraysSupport::hugeLength #17043

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8247373: ArraysSupport.newLength doc, test, and exception message #1617

8247373: ArraysSupport.newLength doc, test, and exception message #1617

stuart-marks commented Dec 4, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

mlbridge bot commented Dec 4, 2020 •

edited

Loading

mlbridge bot commented Dec 4, 2020

RogerRiggs left a comment

RogerRiggs Dec 4, 2020

stuart-marks Dec 4, 2020 •

edited

Loading

stefan-zobel Dec 4, 2020 •

edited by bridgekeeper bot

Loading

stuart-marks commented Dec 5, 2020 •

edited

Loading

openjdk bot commented Dec 8, 2020 •

edited

Loading

stuart-marks commented Dec 17, 2020

bridgekeeper bot commented Jan 14, 2021

Martin-Buchholz left a comment

Martin-Buchholz Jan 24, 2021

Martin-Buchholz commented Jan 24, 2021

bridgekeeper bot commented Feb 21, 2021

pavelrappo left a comment

stuart-marks commented Mar 2, 2021

openjdk bot commented Mar 2, 2021

8247373: ArraysSupport.newLength doc, test, and exception message #1617

8247373: ArraysSupport.newLength doc, test, and exception message #1617

Conversation

stuart-marks commented Dec 4, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

mlbridge bot commented Dec 4, 2020 • edited Loading

Webrevs

mlbridge bot commented Dec 4, 2020

RogerRiggs left a comment

Choose a reason for hiding this comment

RogerRiggs Dec 4, 2020

Choose a reason for hiding this comment

stuart-marks Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

stefan-zobel Dec 4, 2020 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

stuart-marks commented Dec 5, 2020 • edited Loading

openjdk bot commented Dec 8, 2020 • edited Loading

stuart-marks commented Dec 17, 2020

bridgekeeper bot commented Jan 14, 2021

Martin-Buchholz left a comment

Choose a reason for hiding this comment

Martin-Buchholz Jan 24, 2021

Choose a reason for hiding this comment

Martin-Buchholz commented Jan 24, 2021

bridgekeeper bot commented Feb 21, 2021

pavelrappo left a comment

Choose a reason for hiding this comment

stuart-marks commented Mar 2, 2021

openjdk bot commented Mar 2, 2021

stuart-marks commented Dec 4, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 4, 2020 •

edited

Loading

stuart-marks Dec 4, 2020 •

edited

Loading

stefan-zobel Dec 4, 2020 •

edited by bridgekeeper bot

Loading

stuart-marks commented Dec 5, 2020 •

edited

Loading

openjdk bot commented Dec 8, 2020 •

edited

Loading