8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying #4487

turbanoff · 2021-06-14T17:00:29Z

I found few places, where code initially perform Object[] Colleciton.toArray() call and then manually copy array into another array with required type.
This PR cleanups such places to more shorter call T[] Collection.toArray(T[]).

Progress

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

Issue

JDK-8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying

Reviewers

Sean Mullan (@seanjmullan - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4487

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

Using diff file

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

…o avoid redundant array copying

bridgekeeper · 2021-06-14T17:01:13Z

👋 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-06-14T17:03:07Z

@turbanoff The following labels will be automatically applied to this pull request:

2d
awt
core-libs
security
serviceability

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

…o avoid redundant array copying update copyright.

src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java

…o avoid redundant array copying fix incorrect conversion: previously the code returns values

mbien · 2021-06-15T12:06:43Z

src/java.base/share/classes/java/security/Security.java

-        }
-
-        return result;
+        return candidates.toArray(new Provider[0]);


candidates.toArray(new Provider[candidates.size()]);

would use the fast path of the toArray() implementation. Performance probably doesn't matter much in a lot of this cases, but since its only a few characters more, its still worth considering IMO.

The only places I would leave this out are on the HashTable and on the Vector collections in this PR, since calling one synchronized method is not the same as calling two - concurrency wise.

(i am no reviewer)

Actually it's not the fast path - see https://shipilev.net/blog/2016/arrays-wisdom-ancients/

oh I am sorry my mistake - I actually now remember reading the article. (It would be interesting to rerun the benchmark on modern JDKs since I would expect the gap to be smaller now)
Please disregard my suggestion.

stsypanov · 2021-06-22T11:57:20Z

I think the same simlification can be done for some classes affected by your previous PR #4482, e.g. HttpsClient, lines 154-157 and 177-180 and PKCS7, so I'd wait for #4482 and then add one more commit here.

stsypanov · 2021-06-22T12:14:00Z

@turbanoff I've filed a ticket for this: https://bugs.openjdk.java.net/browse/JDK-8269130. Also I think you can integrate #4482

bridgekeeper · 2021-07-20T12:31:13Z

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

mlbridge · 2021-07-26T11:30:20Z

Webrevs

00: Full (d04cd36c)

seanjmullan

The changes to the security classes look good.

openjdk · 2021-07-26T19:09:06Z

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

8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying

Reviewed-by: mullan, serb

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

38ff85c: 8271461: CompileCommand support for hidden class methods
c495ede: 8272099: mark hotspot runtime/Monitor tests which ignore external VM flags
e882087: 8271904: mark hotspot runtime/ClassFile tests which ignore external VM flags
fa36e33: 8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator
cc61520: 8270842: G1: Only young regions need to redirty outside references in remset.
f4cf2f7: 8272095: ProblemList java/nio/channels/FileChannel/Transfer2GPlus.java on linux-aarch64
0aca4f7: 8271868: Warn user when using mac-sign option with unsigned app-image.
b6a19f1: 8271128: InlineIntrinsics support for 32-bit ARM
c2b7fac: 8271896: Remove unnecessary top address checks in BOT
e7b6f48: 8265602: -XX:DumpLoadedClassList should support custom loaders
... and 704 more: https://git.openjdk.java.net/jdk/compare/17295b1bb02b2121978f1459b2e75c5e1031e7ea...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 (@seanjmullan, @mrserb) 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).

bokken · 2021-07-26T19:15:55Z

src/java.base/share/classes/java/security/Security.java

-        }
-
-        return result;
+        return candidates.toArray(new Provider[0]);


Is this called often enough to warrant creating a constant of new Provider[0] (benefits of this covered in the Wisdom of the Ancients blog linked)?

May be yes, may be not. I like 0-sized array approach more.

It looks clearer and easier to read

It should be faster than original code anyway

bokken · 2021-07-26T19:55:09Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/SystemDictionaryHelper.java

-      Object[] tmpArray = tmp.toArray();
-      InstanceKlass[] searchResult = new InstanceKlass[tmpArray.length];
-      System.arraycopy(tmpArray, 0, searchResult, 0, tmpArray.length);
+      InstanceKlass[] searchResult = tmp.toArray(new InstanceKlass[0]);


Is it too far outside the scope of these changes to make tmp an ArrayList rather than a Vector?

I'll create separate PRs to migrate Vector usages to ArrayList.

mrserb

Changes in the desktop module looks fine.

turbanoff · 2021-08-09T07:19:54Z

/integrate

openjdk · 2021-08-09T07:21:06Z

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

jayathirthrao · 2021-08-10T13:26:55Z

/sponsor

openjdk · 2021-08-10T13:28:06Z

Going to push as commit 35b399a.
Since your change was applied there have been 735 commits pushed to the master branch:

2b05fae: 8260262: Use common code in function unmap_shared() in perfMemory_posix.cpp
f2599ad: 8272196: Remove unused class ParStrongRootsScope
1f88134: 8271869: AArch64: build errors with GCC11 in frame::saved_oop_result
089e83b: 8266490: Extend the OSContainer API to support the pids controller of cgroups
2384e12: 8270098: ZGC: ZBarrierSetC2::clone_at_expansion fails with "Guard against surprises" assert
d53d94b: 8271925: ZGC: Arraycopy stub passes invalid oop to load barrier
3b899ef: 8272168: some hotspot runtime/logging tests don't check exit code
abdc107: 8270454: G1: Simplify region index comparison
eb6f3fe: 8272169: runtime/logging/LoaderConstraintsTest.java doesn't build test.Empty
9654fd7: 8271892: mark hotspot runtime/PrintStringTableStats/PrintStringTableStatsTest.java test as ignoring external VM flags
... and 725 more: https://git.openjdk.java.net/jdk/compare/17295b1bb02b2121978f1459b2e75c5e1031e7ea...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-10T13:28:25Z

@jayathirthrao @turbanoff Pushed as commit 35b399a.

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

Replace usages of Collection.toArray() with Collection.toArray(T[]) t…

ba73349

…o avoid redundant array copying

openjdk bot added serviceability serviceability-dev@openjdk.org security security-dev@openjdk.org 2d client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Jun 14, 2021

Replace usages of Collection.toArray() with Collection.toArray(T[]) t…

5aa9e8e

…o avoid redundant array copying update copyright.

mrserb reviewed Jun 14, 2021

View reviewed changes

src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java Outdated Show resolved Hide resolved

Replace usages of Collection.toArray() with Collection.toArray(T[]) t…

d04cd36

…o avoid redundant array copying fix incorrect conversion: previously the code returns values

mbien reviewed Jun 15, 2021

View reviewed changes

turbanoff changed the title ~~[PATCH] Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying~~ 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying Jul 26, 2021

openjdk bot added the rfr Pull request is ready for review label Jul 26, 2021

seanjmullan approved these changes Jul 26, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 26, 2021

bokken reviewed Jul 26, 2021

View reviewed changes

mrserb approved these changes Aug 2, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 9, 2021

openjdk bot closed this Aug 10, 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 Aug 10, 2021

turbanoff deleted the use_generic_collection_to_array_to_avoid_redundant_allocations branch September 15, 2021 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying #4487

8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying #4487

turbanoff commented Jun 14, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jun 14, 2021

openjdk bot commented Jun 14, 2021

mbien Jun 15, 2021

turbanoff Jun 15, 2021

mbien Jun 15, 2021

stsypanov commented Jun 22, 2021

stsypanov commented Jun 22, 2021

bridgekeeper bot commented Jul 20, 2021

mlbridge bot commented Jul 26, 2021

seanjmullan left a comment

openjdk bot commented Jul 26, 2021 •

edited

bokken Jul 26, 2021

turbanoff Aug 9, 2021

bokken Jul 26, 2021 •

edited

turbanoff Aug 9, 2021

mrserb left a comment

turbanoff commented Aug 9, 2021

openjdk bot commented Aug 9, 2021

jayathirthrao commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying #4487

8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying #4487

Conversation

turbanoff commented Jun 14, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 14, 2021

openjdk bot commented Jun 14, 2021

mbien Jun 15, 2021

Choose a reason for hiding this comment

turbanoff Jun 15, 2021

Choose a reason for hiding this comment

mbien Jun 15, 2021

Choose a reason for hiding this comment

stsypanov commented Jun 22, 2021

stsypanov commented Jun 22, 2021

bridgekeeper bot commented Jul 20, 2021

mlbridge bot commented Jul 26, 2021

Webrevs

seanjmullan left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 26, 2021 • edited

bokken Jul 26, 2021

Choose a reason for hiding this comment

turbanoff Aug 9, 2021

Choose a reason for hiding this comment

bokken Jul 26, 2021 • edited

Choose a reason for hiding this comment

turbanoff Aug 9, 2021

Choose a reason for hiding this comment

mrserb left a comment

Choose a reason for hiding this comment

turbanoff commented Aug 9, 2021

openjdk bot commented Aug 9, 2021

jayathirthrao commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

turbanoff commented Jun 14, 2021 •

edited by openjdk bot

openjdk bot commented Jul 26, 2021 •

edited

bokken Jul 26, 2021 •

edited