8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE #5878

turbanoff · 2021-10-09T17:54:16Z

Progress

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

Issue

JDK-8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE

Reviewers

Pavel Rappo (@pavelrappo - Reviewer)
Jim Laskey (@JimLaskey - Reviewer) ⚠️ Review applies to d679bd3
Martin Buchholz (@Martin-Buchholz - Reviewer) ⚠️ Review applies to 4ba785b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5878/head:pull/5878
$ git checkout pull/5878

Update a local copy of the PR:
$ git checkout pull/5878
$ git pull https://git.openjdk.java.net/jdk pull/5878/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5878

View PR using the GUI difftool:
$ git pr show -t 5878

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5878.diff

bridgekeeper · 2021-10-09T17:55:26Z

👋 Welcome back turbanoff! 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 · 2021-10-09T17:57:34Z

@turbanoff 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 · 2021-10-09T19:40:17Z

Webrevs

openjdk · 2021-10-10T17:11:18Z

@turbanoff 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:

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE

Reviewed-by: prappo, jlaskey, martin

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 24 new commits pushed to the master branch:

cfe7471: 8177814: jdk/editpad is not in jdk TEST.groups
a5f09d1: 8275031: runtime/ErrorHandling/MachCodeFramesInErrorFile.java fails when hsdis is present
ef0922e: 8274560: JFR: Add test for OldObjectSample event when using Shenandoah
1e30695: 8274466: G1: use field directly rather than method in G1CollectorState::in_mixed_phase
dd93c6e: 8272167: AbsPathsInImage.java should skip *.dSYM directories
829dea4: 8274945: Cleanup unnecessary calls to Throwable.initCause() in java.desktop
75f5145: 8274925: Shenandoah: shenandoah/TestAllocHumongousFragment.java test failed on lock rank check
83c3771: 8273881: Metaspace: test repeated deallocations
3f01d03: 8275021: Test serviceability/sa/TestJmapCore.java fails with: java.io.IOException: Stack frame 0x4 not found
3f07337: 8273614: Shenandoah: intermittent timeout with ConcurrentGCBreakpoint tests
... and 14 more: https://git.openjdk.java.net/jdk/compare/6d1d4d52928ed38bbc73ddcbede5389995a8e65f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@pavelrappo, @JimLaskey, @Martin-Buchholz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

pavelrappo · 2021-10-10T19:34:05Z

Oops. I think we should also do something about this occurrence of MAX_ARRAY_SIZE:

jdk/src/java.base/share/classes/java/lang/AbstractStringBuilder.java

Line 239 in d679bd3

* {@code (MAX_ARRAY_SIZE >> coder)} unless the given minimum capacity

src/java.base/share/classes/java/lang/AbstractStringBuilder.java

Martin-Buchholz

JDK sources should not contain dead unused fields - thanks for fixing.

The change to use newLength in this file should have adjusted the javadoc of newCapacity, perhaps simply to refer to ArraysSupport.SOFT_MAX_ARRAY_LENGTH instead.

That sounds like a job for Jim Laskey as the author of
commit 03642a0
Author: Jim Laskey jlaskey@openjdk.org
Date: Thu Jun 11 10:08:23 2020 -0300

8230744: Several classes throw OutOfMemoryError without message

Reviewed-by: psandoz, martin, bchristi, rriggs, smarks

If that is fixed (perhaps in a different commit), then this commit is good.

History has shown that capacity growth code is highly errorprone, so it's worth writing whitebox tests, as I did in e.g.

./java/util/concurrent/ConcurrentHashMap/WhiteBox.java
./java/util/ArrayDeque/WhiteBox.java
./java/util/HashMap/WhiteBoxResizeTest.java

update javadoc of 'newCapacity' method to refer ArraysSupport.SOFT_MAX_ARRAY_LENGTH instead

pavelrappo · 2021-10-11T18:57:53Z

I'll run tests; if they pass, I'll sponsor the change.

Martin-Buchholz

We generally avoid in javadoc. Especially in private javadoc. I would write this more simply/readably as



{@code SOFT_MAX_ARRAY_LENGTH >> coder}

avoid link in private javadoc

turbanoff · 2021-10-12T06:32:45Z

/integrate

openjdk · 2021-10-12T06:34:15Z

@turbanoff
Your change (at version d35afde) is now ready to be sponsored by a Committer.

pavelrappo · 2021-10-12T09:35:48Z

/sponsor

openjdk · 2021-10-12T09:38:01Z

Going to push as commit 7d2633f.
Since your change was applied there have been 24 commits pushed to the master branch:

cfe7471: 8177814: jdk/editpad is not in jdk TEST.groups
a5f09d1: 8275031: runtime/ErrorHandling/MachCodeFramesInErrorFile.java fails when hsdis is present
ef0922e: 8274560: JFR: Add test for OldObjectSample event when using Shenandoah
1e30695: 8274466: G1: use field directly rather than method in G1CollectorState::in_mixed_phase
dd93c6e: 8272167: AbsPathsInImage.java should skip *.dSYM directories
829dea4: 8274945: Cleanup unnecessary calls to Throwable.initCause() in java.desktop
75f5145: 8274925: Shenandoah: shenandoah/TestAllocHumongousFragment.java test failed on lock rank check
83c3771: 8273881: Metaspace: test repeated deallocations
3f01d03: 8275021: Test serviceability/sa/TestJmapCore.java fails with: java.io.IOException: Stack frame 0x4 not found
3f07337: 8273614: Shenandoah: intermittent timeout with ConcurrentGCBreakpoint tests
... and 14 more: https://git.openjdk.java.net/jdk/compare/6d1d4d52928ed38bbc73ddcbede5389995a8e65f...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-12T09:38:21Z

@pavelrappo @turbanoff Pushed as commit 7d2633f.

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

[PATCH] Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE

d679bd3

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 9, 2021

turbanoff changed the title ~~[PATCH] Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE~~ 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE Oct 9, 2021

openjdk bot added the rfr Pull request is ready for review label Oct 9, 2021

pavelrappo approved these changes Oct 10, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2021

JimLaskey approved these changes Oct 10, 2021

View reviewed changes

mrserb reviewed Oct 10, 2021

View reviewed changes

src/java.base/share/classes/java/lang/AbstractStringBuilder.java Show resolved Hide resolved

Martin-Buchholz approved these changes Oct 11, 2021

View reviewed changes

275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE

4ba785b

update javadoc of 'newCapacity' method to refer ArraysSupport.SOFT_MAX_ARRAY_LENGTH instead

Martin-Buchholz approved these changes Oct 11, 2021

View reviewed changes

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE

d35afde

avoid link in private javadoc

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 12, 2021

pavelrappo approved these changes Oct 12, 2021

View reviewed changes

openjdk bot closed this Oct 12, 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 sponsor Pull request is ready to be sponsored labels Oct 12, 2021

turbanoff deleted the remove_unused_AbstractStringBuilder.MAX_ARRAY_SIZE branch February 21, 2022 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE #5878

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE #5878

Uh oh!

turbanoff commented Oct 9, 2021 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Oct 9, 2021

Uh oh!

openjdk bot commented Oct 9, 2021

Uh oh!

mlbridge bot commented Oct 9, 2021 •

edited

Loading

Uh oh!

openjdk bot commented Oct 10, 2021 •

edited

Loading

Uh oh!

pavelrappo commented Oct 10, 2021

Uh oh!

Uh oh!

Martin-Buchholz left a comment

Uh oh!

pavelrappo commented Oct 11, 2021

Uh oh!

Martin-Buchholz left a comment

Uh oh!

turbanoff commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

pavelrappo commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

Uh oh!

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE #5878

8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE #5878

Uh oh!

Conversation

turbanoff commented Oct 9, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 9, 2021

Uh oh!

openjdk bot commented Oct 9, 2021

Uh oh!

mlbridge bot commented Oct 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavelrappo commented Oct 10, 2021

Uh oh!

Uh oh!

Martin-Buchholz left a comment

Choose a reason for hiding this comment

Uh oh!

pavelrappo commented Oct 11, 2021

Uh oh!

Martin-Buchholz left a comment

Choose a reason for hiding this comment

Uh oh!

turbanoff commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

pavelrappo commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

openjdk bot commented Oct 12, 2021

Uh oh!

Uh oh!

turbanoff commented Oct 9, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 9, 2021 •

edited

Loading

openjdk bot commented Oct 10, 2021 •

edited

Loading