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

8263561: Re-examine uses of LinkedList #2744

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -40,7 +40,6 @@
import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
@@ -242,7 +241,7 @@ private static void checkReturnTypes(List<ProxyMethod> methods) {
* List of return types that are not yet known to be
* assignable from ("covered" by) any of the others.
*/
LinkedList<Class<?>> uncoveredReturnTypes = new LinkedList<>();
List<Class<?>> uncoveredReturnTypes = new ArrayList<>(1);

nextNewReturnType:
for (ProxyMethod pm : methods) {
@@ -2887,7 +2887,7 @@ public static final Control getNoFallbackControl(List<String> formats) {
if (language.equals("nb") || isNorwegianBokmal) {
List<Locale> tmpList = getDefaultList("nb", script, region, variant);
// Insert a locale replacing "nb" with "no" for every list entry with precedence
List<Locale> bokmalList = new LinkedList<>();
List<Locale> bokmalList = new ArrayList<>();
for (Locale l_nb : tmpList) {
var isRoot = l_nb.getLanguage().isEmpty();
var l_no = Locale.getInstance(isRoot ? "" : "no",
@@ -2934,15 +2934,15 @@ else if (language.equals("zh")) {
List<String> variants = null;

if (!variant.isEmpty()) {
variants = new LinkedList<>();
variants = new ArrayList<>();
int idx = variant.length();
while (idx != -1) {
variants.add(variant.substring(0, idx));
idx = variant.lastIndexOf('_', --idx);
}
}

List<Locale> list = new LinkedList<>();
List<Locale> list = new ArrayList<>();

if (variants != null) {
for (String v : variants) {
@@ -54,7 +54,6 @@
import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Properties;
@@ -218,7 +217,7 @@ public URLClassPath(URL[] urls, AccessControlContext acc) {
if (closed) {
return Collections.emptyList();
}
List<IOException> result = new LinkedList<>();
List<IOException> result = new ArrayList<>();
This conversation was marked as resolved by stsypanov

This comment has been minimized.

@kelthuzadx

kelthuzadx Mar 14, 2021
Member

We'd better be cautious about this replacement since its caller will remove the first element of this array, that's one of the scenarios where LinkedList usually has better performance than ArrayList.

Just IMHO, I suggest replacing them only if there is a performance improvement(e.g. benchmark reports). Changing field types will break users' existing application code, they might reflectively modify these values.

This comment has been minimized.

@liach

liach Mar 14, 2021
Contributor

If that's the only use case, I recommend changing the return type to a deque, and replace the linked list with an array deque instead (as done elsewhere in this pr)

This comment has been minimized.

@stsypanov

stsypanov Mar 14, 2021
Author Contributor

Looks like it's never specified in JavaDoc which particular implementation of List is used in fields of affected classes, so it's quite odd to me that someone would rely on that when using reflection. But your point about backward compatibility is reasonable, so I'll revert mentioned changes.

This comment has been minimized.

@AlanBateman

AlanBateman Mar 14, 2021
Contributor

Nothing outside of the JDK should be hacking into this private field of a non-exposed class, this should not be a concern here.

This comment has been minimized.

@stsypanov

stsypanov Mar 14, 2021
Author Contributor

@AlanBateman so is it ok to keep ArrayLists?

This comment has been minimized.

@AlanBateman

AlanBateman Mar 15, 2021
Contributor

@AlanBateman so is it ok to keep ArrayLists?

One thing to look out for is usages of 2-arg add method that inserts at a specific position. This shouldn't be a concern in URLClassPath.closeLoaders (assuming this is this usage where the question arises).

This comment has been minimized.

@stsypanov

stsypanov Apr 8, 2021
Author Contributor

@AlanBateman looks like List.add(o, i) is not used at all in scope of these changes.

for (Loader loader : loaders) {
try {
loader.close();
@@ -954,7 +953,7 @@ Resource getResource(final String name, boolean check,
Resource res;
String[] jarFiles;
int count = 0;
LinkedList<String> jarFilesList = null;
List<String> jarFilesList;

/* If there no jar files in the index that can potential contain
* this resource then return immediately.
@@ -52,13 +52,13 @@
* The hash map that maintains mappings from
* package/classe/resource to jar file list(s)
*/
private HashMap<String,LinkedList<String>> indexMap;
private final HashMap<String, List<String>> indexMap;

/**
* The hash map that maintains mappings from
* jar file to package/class/resource lists
*/
private HashMap<String,LinkedList<String>> jarMap;
private final HashMap<String, List<String>> jarMap;

/*
* An ordered list of jar file names.
@@ -132,13 +132,13 @@ public static JarIndex getJarIndex(JarFile jar) throws IOException {

/*
* Add the key, value pair to the hashmap, the value will
* be put in a linked list which is created if necessary.
* be put in a list which is created if necessary.
*/
private void addToList(String key, String value,
HashMap<String,LinkedList<String>> t) {
LinkedList<String> list = t.get(key);
HashMap<String, List<String>> t) {
List<String> list = t.get(key);
if (list == null) {
list = new LinkedList<>();
list = new ArrayList<>(1);
list.add(value);
t.put(key, list);
} else if (!list.contains(value)) {
@@ -151,8 +151,8 @@ private void addToList(String key, String value,
*
* @param fileName the key of the mapping
*/
public LinkedList<String> get(String fileName) {
LinkedList<String> jarFiles = null;
public List<String> get(String fileName) {

This comment has been minimized.

@AlanBateman

AlanBateman May 21, 2021
Contributor

IcedTea-Web seems to be using this method reflectively:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80

I assume this doesn't work with JDK 16, at least not without using --add-exports to export jdk.internal.util.jar.

This comment has been minimized.

This comment has been minimized.

@stsypanov

stsypanov May 24, 2021
Author Contributor

Should we then revert the changes to JarIndex?

This comment has been minimized.

@liach

liach May 24, 2021
Contributor

But don't people access these internal code at their own risk, as jdk may change these code at any time without notice?

This comment has been minimized.

@stsypanov

stsypanov May 24, 2021
Author Contributor

True, I just wonder whether it's OK to change internals when we know for sure that this breaks 3rd party code

This comment has been minimized.

@liach

liach May 24, 2021
Contributor

I think so. There are always unexpected ways the jdk may break and third-party libraries would need a different workaround for a new java version. For instance, in Apache log4j, its library has a special guard against the broken implementation of Reflection getCallerClass during java 7. The libraries can just handle these in their version-specific components.

Especially given the fact that the code linked above already has version-specific handling (8 vs 9), so it won't hurt much for them to add another piece of logic to handle jdk 17+ in case this optimization is merged.

List<String> jarFiles;
if ((jarFiles = indexMap.get(fileName)) == null) {
/* try the package name again */
int pos;
@@ -166,7 +166,7 @@ private void addToList(String key, String value,
/**
* Add the mapping from the specified file to the specified
* jar file. If there were no mapping for the package of the
* specified file before, a new linked list will be created,
* specified file before, a new list will be created,
* the jar file is added to the list and a new mapping from
* the package to the jar file list is added to the hashmap.
* Otherwise, the jar file will be added to the end of the
@@ -261,7 +261,7 @@ public void write(OutputStream out) throws IOException {
/* print out the jar file name */
String jar = jarFiles[i];
bw.write(jar + "\n");
LinkedList<String> jarlist = jarMap.get(jar);
List<String> jarlist = jarMap.get(jar);
if (jarlist != null) {
Iterator<String> listitr = jarlist.iterator();
while(listitr.hasNext()) {
@@ -288,7 +288,7 @@ public void read(InputStream is) throws IOException {
String currentJar = null;

/* an ordered list of jar file names */
Vector<String> jars = new Vector<>();
List<String> jars = new ArrayList<>();

/* read until we see a .jar line */
while((line = br.readLine()) != null && !line.endsWith(".jar"));
@@ -320,11 +320,11 @@ public void read(InputStream is) throws IOException {
*
*/
public void merge(JarIndex toIndex, String path) {
Iterator<Map.Entry<String,LinkedList<String>>> itr = indexMap.entrySet().iterator();
Iterator<Map.Entry<String, List<String>>> itr = indexMap.entrySet().iterator();
while(itr.hasNext()) {
Map.Entry<String,LinkedList<String>> e = itr.next();
Map.Entry<String, List<String>> e = itr.next();
String packageName = e.getKey();
LinkedList<String> from_list = e.getValue();
List<String> from_list = e.getValue();
Iterator<String> listItr = from_list.iterator();
while(listItr.hasNext()) {
String jarName = listItr.next();
@@ -30,7 +30,7 @@
import java.nio.channels.MembershipKey;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

@@ -94,7 +94,7 @@ void add(MembershipKeyImpl key) {
keys = groups.get(group);
}
if (keys == null) {
keys = new LinkedList<>();
keys = new ArrayList<>();
groups.put(group, keys);
}
keys.add(key);
@@ -40,14 +40,14 @@

abstract class AbstractPoller implements Runnable {

// list of requests pending to the poller thread
private final LinkedList<Request> requestList;
// requests pending to the poller thread
private final ArrayDeque<Request> requests;

// set to true when shutdown
private boolean shutdown;

protected AbstractPoller() {
this.requestList = new LinkedList<>();
this.requests = new ArrayDeque<>();
this.shutdown = false;
}

@@ -215,11 +215,11 @@ Object awaitResult() {
private Object invoke(RequestType type, Object... params) throws IOException {
// submit request
Request req = new Request(type, params);
synchronized (requestList) {
synchronized (requests) {
if (shutdown) {
throw new ClosedWatchServiceException();
}
requestList.add(req);
requests.add(req);

// wakeup thread
wakeup();
@@ -242,9 +242,9 @@ private Object invoke(RequestType type, Object... params) throws IOException {
*/
@SuppressWarnings("unchecked")
boolean processRequests() {
synchronized (requestList) {
synchronized (requests) {
Request req;
while ((req = requestList.poll()) != null) {
while ((req = requests.poll()) != null) {
// if in process of shutdown then reject request
if (shutdown) {
req.release(new ClosedWatchServiceException());
@@ -26,7 +26,7 @@
package sun.util.locale.provider;

import java.lang.ref.SoftReference;
import java.util.LinkedList;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -98,7 +98,7 @@

// Performs per-ID retrieval.
Set<String> zoneIDs = LocaleProviderAdapter.forJRE().getLocaleResources(locale).getZoneIDs();
List<String[]> zones = new LinkedList<>();
List<String[]> zones = new ArrayList<>();
for (String key : zoneIDs) {
String[] names = retrieveDisplayNamesImpl(key, locale);
if (names != null) {
@@ -26,7 +26,7 @@
package sun.net.dns;

import java.util.List;
import java.util.LinkedList;
import java.util.ArrayList;
import java.util.StringTokenizer;
import java.io.BufferedReader;
import java.io.FileReader;
@@ -56,11 +56,11 @@
// Parse /etc/resolv.conf to get the values for a particular
// keyword.
//
private LinkedList<String> resolvconf(String keyword,
private ArrayList<String> resolvconf(String keyword,
int maxperkeyword,
int maxkeywords)
{
LinkedList<String> ll = new LinkedList<>();
ArrayList<String> ll = new ArrayList<>();

try {
BufferedReader in =
@@ -111,8 +111,8 @@
return ll;
}

private LinkedList<String> searchlist;
private LinkedList<String> nameservers;
private ArrayList<String> searchlist;
private ArrayList<String> nameservers;


// Load DNS configuration from OS
@@ -132,7 +132,7 @@ private void loadConfig() {
nameservers =
java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction<>() {
public LinkedList<String> run() {
public ArrayList<String> run() {
// typically MAXNS is 3 but we've picked 5 here
// to allow for additional servers if required.
return resolvconf("nameserver", 1, 5);
@@ -149,16 +149,16 @@ private void loadConfig() {

// obtain search list or local domain

private LinkedList<String> getSearchList() {
private ArrayList<String> getSearchList() {

LinkedList<String> sl;
ArrayList<String> sl;

// first try the search keyword in /etc/resolv.conf

sl = java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction<>() {
public LinkedList<String> run() {
LinkedList<String> ll;
public ArrayList<String> run() {
ArrayList<String> ll;

// first try search keyword (max 6 domains)
ll = resolvconf("search", 6, 1);
@@ -181,8 +181,8 @@ private void loadConfig() {

sl = java.security.AccessController.doPrivileged(
new java.security.PrivilegedAction<>() {
public LinkedList<String> run() {
LinkedList<String> ll;
public ArrayList<String> run() {
ArrayList<String> ll;

ll = resolvconf("domain", 1, 1);
if (ll.size() > 0) {
@@ -199,7 +199,7 @@ private void loadConfig() {
// no local domain so try fallback (RPC) domain or
// hostName

sl = new LinkedList<>();
sl = new ArrayList<>();
String domain = fallbackDomain0();
if (domain != null && !domain.isEmpty()) {
sl.add(domain);
@@ -35,7 +35,7 @@
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.LinkedList;
import java.util.List;
import java.util.jar.JarEntry;
import java.util.jar.JarOutputStream;
// implementation specific API
@@ -64,7 +64,7 @@ public static void main(String[] args) throws Exception {
static void assertFileResolved(JarIndex jarIndex2, String file,
String jarName) {
@SuppressWarnings("unchecked")
LinkedList<String> jarLists = (LinkedList<String>)jarIndex2.get(file);
List<String> jarLists = (List<String>)jarIndex2.get(file);
if (jarLists == null || jarLists.size() == 0 ||
!jarName.equals(jarLists.get(0))) {
throw new RuntimeException(