Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -653,14 +653,7 @@ public static Provider[] getProviders(Map<String,String> filter) {
if (candidates == null || candidates.isEmpty())
return null;

Object[] candidatesArray = candidates.toArray();
Provider[] result = new Provider[candidatesArray.length];

for (int i = 0; i < result.length; i++) {
result[i] = (Provider)candidatesArray[i];
}

return result;
return candidates.toArray(new Provider[0]);
Copy link
Contributor

@mbien mbien Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@turbanoff turbanoff Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mbien mbien Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@bokken bokken Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

@turbanoff turbanoff Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. It looks clearer and easier to read
  2. It should be faster than original code anyway

}

// Map containing cached Spi Class objects of the specified type
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2006, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -241,10 +241,8 @@ public void encode(byte tag, OutputStream out) throws IOException {

private byte[] generateDerEncoding() throws IOException {
DerOutputStream out = new DerOutputStream();
Object[] attribVals = attributes.values().toArray();

out.putOrderedSetOf(DerValue.tag_SetOf,
castToDerEncoder(attribVals));
DerEncoder[] attribVals = attributes.values().toArray(new DerEncoder[0]);
out.putOrderedSetOf(DerValue.tag_SetOf, attribVals);
return out.toByteArray();
}

@@ -348,17 +346,4 @@ public String toString() {
return sb.toString();
}

/**
* Cast an object array whose components are
* <code>DerEncoder</code>s to <code>DerEncoder[]</code>.
*/
static DerEncoder[] castToDerEncoder(Object[] objs) {

DerEncoder[] encoders = new DerEncoder[objs.length];

for (int i=0; i < encoders.length; i++)
encoders[i] = (DerEncoder) objs[i];

return encoders;
}
}
@@ -3352,14 +3352,7 @@ public Font[] getAllInstalledFonts() {
}
}

String[] fontNames = null;
if (fontMapNames.size() > 0) {
fontNames = new String[fontMapNames.size()];
Object [] keyNames = fontMapNames.keySet().toArray();
for (int i=0; i < keyNames.length; i++) {
fontNames[i] = (String)keyNames[i];
}
}
String[] fontNames = fontMapNames.keySet().toArray(new String[0]);
Font[] fonts = new Font[fontNames.length];
for (int i=0; i < fontNames.length; i++) {
fonts[i] = new Font(fontNames[i], Font.PLAIN, 1);
@@ -3428,11 +3421,7 @@ public String[] getInstalledFontFamilyNames(Locale requestedLocale) {
// Add any native font family names here
addNativeFontFamilyNames(familyNames, requestedLocale);

String[] retval = new String[familyNames.size()];
Object [] keyNames = familyNames.keySet().toArray();
for (int i=0; i < keyNames.length; i++) {
retval[i] = familyNames.get(keyNames[i]);
}
String[] retval = familyNames.values().toArray(new String[0]);
if (requestedLocale.equals(Locale.getDefault())) {
lastDefaultLocale = requestedLocale;
allFamilies = new String[retval.length];
@@ -188,11 +188,7 @@ public String[] getAvailableFontFamilyNames(Locale requestedLocale) {
map.put(installed[i].toLowerCase(requestedLocale),
installed[i]);
}
String[] retval = new String[map.size()];
Object [] keyNames = map.keySet().toArray();
for (int i=0; i < keyNames.length; i++) {
retval[i] = map.get(keyNames[i]);
}
String[] retval = map.values().toArray(new String[0]);
return retval;
}
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -89,12 +89,7 @@ static boolean isWindowsProperty(String name) {
* Returns String[] containing available property names
*/
private String [] getKeyNames() {
Object[] keys = map.keySet().toArray();
String[] sortedKeys = new String[keys.length];

for ( int nkey = 0; nkey < keys.length; nkey++ ) {
sortedKeys[nkey] = keys[nkey].toString();
}
String[] sortedKeys = map.keySet().toArray(new String[0]);
Arrays.sort(sortedKeys);
return sortedKeys;
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2012, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -427,13 +427,7 @@ public Collection getRowSets() throws SQLException {
* @see CachedRowSet#setTableName
*/
public String[] getRowSetNames() throws SQLException {
Object [] arr = vecTableNames.toArray();
String []strArr = new String[arr.length];

for( int i = 0;i < arr.length; i++) {
strArr[i] = arr[i].toString();
}

String[] strArr = vecTableNames.toArray(new String[0]);
return strArr;
}

@@ -234,8 +234,7 @@ private void initJFrameCache() {
// after printing stack trace.
exp.printStackTrace();
}
JavaVFrame[] jvframes = new JavaVFrame[tmp.size()];
System.arraycopy(tmp.toArray(), 0, jvframes, 0, jvframes.length);
JavaVFrame[] jvframes = tmp.toArray(new JavaVFrame[0]);
jframeCache.put(cur.getThreadProxy(), jvframes);
proxyToThread.put(cur.getThreadProxy(), cur);
}
@@ -286,8 +285,7 @@ private String[] getJavaNames(ThreadProxy th, Address fp) {
names.add(sb.toString());
}
}
String[] res = new String[names.size()];
System.arraycopy(names.toArray(), 0, res, 0, res.length);
String[] res = names.toArray(new String[0]);
return res;
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2002, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -65,9 +65,7 @@ public void visit(Klass k) {
}
});

Object[] tmpArray = tmp.toArray();
klasses = new InstanceKlass[tmpArray.length];
System.arraycopy(tmpArray, 0, klasses, 0, tmpArray.length);
klasses = tmp.toArray(new InstanceKlass[0]);
Arrays.sort(klasses, new Comparator<>() {
public int compare(InstanceKlass k1, InstanceKlass k2) {
Symbol s1 = k1.getName();
@@ -91,9 +89,7 @@ public static InstanceKlass[] findInstanceKlasses(String namePart) {
}
}

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]);
Copy link

@bokken bokken Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@turbanoff turbanoff Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return searchResult;
}