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

Conversation

@stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Feb 26, 2021

The usage of LinkedList is senseless and can be replaced with either ArrayList or ArrayDeque which are both more compact and effective.

jdk:tier1 and jdk:tier2 are both ok


Progress

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

Issue

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2744

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 26, 2021

👋 Welcome back stsypanov! 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
Copy link

@openjdk openjdk bot commented Feb 26, 2021

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

  • core-libs
  • i18n
  • net
  • nio

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.

@liach
Copy link
Contributor

@liach liach commented Feb 26, 2021

Are linked lists worse for addition even in cases where addition to array list or deque requires resize and copying? i thought that's the advantage of linked list.

@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented Mar 3, 2021

Are linked lists worse for addition even in cases where addition to array list or deque requires resize and copying? i thought that's the advantage of linked list.

As far as I know LinkedList is always worse than ArrayList and discouraged from being used.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Mar 14, 2021

/issue JDK-8263561

@openjdk
Copy link

@openjdk openjdk bot commented Mar 14, 2021

@AlanBateman Only the author (@stsypanov) is allowed to issue the /issue command.

@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented Mar 14, 2021

/issue JDK-8263561

@openjdk openjdk bot changed the title Remove remaining usages of LinkedList in java.base 8263561: Re-examine uses of LinkedList Mar 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 14, 2021

@stsypanov The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

@openjdk openjdk bot added the rfr label Mar 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 14, 2021

Webrevs

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 6, 2021

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

@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented May 6, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@stsypanov This PR has not yet been marked as ready for integration.

@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented May 21, 2021

Hi guys, any more comments here? Please ping me if I've missed something

Copy link
Member

@dfuch dfuch left a comment

I don't remember all the comments you have received in this thread but have you verified that arbitrarily changing LinkedList into ArrayList in these classes is not going to significantly increase the footprint in the case where lists are empty or contain only one or two elements?

I am not convinced that a global replacement of LinkedList by ArrayList is necessarily good - even though I agree that ArrayList is generally more efficient.

public LinkedList<String> get(String fileName) {
LinkedList<String> jarFiles = null;
public List<String> get(String fileName) {
ArrayList<String> jarFiles;
Copy link
Member

@dfuch dfuch May 21, 2021

Choose a reason for hiding this comment

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

This could probably be declared as:

List<String> jarFiles;

Copy link
Contributor Author

@stsypanov stsypanov May 24, 2021

Choose a reason for hiding this comment

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

Done

@@ -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);
ArrayList<String> jarlist = jarMap.get(jar);
Copy link
Member

@dfuch dfuch May 21, 2021

Choose a reason for hiding this comment

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

Here again, jarList could probably be declared as List<String>

Copy link
Contributor Author

@stsypanov stsypanov May 24, 2021

Choose a reason for hiding this comment

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

Done

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented May 21, 2021

Hi guys, any more comments here? Please ping me if I've missed something

I suspect this will require renaming some fields and changing comments, e.g. requestList is no longer a good name for the field in AbstractPoller, its comments need changes too. The field in ResolverConfigurationImpl is searchList, it will require a few changes. There may be more, I just picked out a few.

@@ -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) {
Copy link

@Thihup Thihup May 21, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

@AlanBateman AlanBateman May 21, 2021

Choose a reason for hiding this comment

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

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.

Copy link

@Thihup Thihup May 21, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@stsypanov stsypanov May 24, 2021

Choose a reason for hiding this comment

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

Should we then revert the changes to JarIndex?

Copy link
Contributor

@liach liach May 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@stsypanov stsypanov May 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@liach liach May 24, 2021

Choose a reason for hiding this comment

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

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.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented May 21, 2021

I second the footprint concerns from @dfuch. I've seen with first hand cases where widespread uses of array lists for holding 1-2-3 elements (e.g. think of a graph where each node might only have a very small number of outgoing edges) lead to massive memory over-utilization. If the average size is known, at the very least I'd argue to replace with an array list which is sized correctly.

And, all this said, the general assumption implied in this PR which linked lists are just to be avoided doesn't match my experience. If you want a "pay only as much memory as you use" data structure, you don't care about random access (e.g. all accesses are linear walks), a linked list is a perfectly fine choice. If there are use cases in the JDK where a LinkedList is used in places where it shouldn't be, then obviously those cases should be replaced.

@stsypanov stsypanov force-pushed the purge-linked-list branch from 73029fe to 158006c May 25, 2021
@stsypanov
Copy link
Contributor Author

@stsypanov stsypanov commented May 25, 2021

Hi @mcimadamore, @dfuch after your review comments I've decided to do a deeper investigation for this. First, I've decided to check whether empty LinkedList is going to have smaller footprint (this could be fruitful for cases when the list is likely to remain empty in vast majority of cases, see e.g. URLClassPath.closeLoaders()), and apparently it isn't:

@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class EmptyListBenchmark {
    @Benchmark
    public Object emptyArrayList() { return new ArrayList<>(); }

    @Benchmark
    public Object emptyLinkedList() { return new LinkedList<>(); }
}

This benchmark with my ad-hoc JDK build yields the following results:

Benchmark                                                            Mode  Cnt     Score     Error   Units
EmptyListBenchmark.emptyArrayList                                    avgt   20     4.605 ±   0.463   ns/op
EmptyListBenchmark.emptyArrayList:·gc.alloc.rate.norm                avgt   20    24.002 ±   0.001    B/op
EmptyListBenchmark.emptyLinkedList                                   avgt   20     4.324 ±   0.081   ns/op
EmptyListBenchmark.emptyLinkedList:·gc.alloc.rate.norm               avgt   20    32.002 ±   0.001    B/op

After JDK-8011200 ArrayList instantiated with default constructor doesn't allocate underlying array any more.

However the things get more complicated when the list gets populated:

@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class NonEmptyListBenchmark {
    @Param({"1", "2", "3", "4", "5"})
    private int size;

    @Benchmark
    public Object arrayList() {
        var list = new ArrayList<>();
        for (int i = 0; i < size; i++) {
            list.add(i);
        }
        return list;
    }

    @Benchmark
    public Object linkedList() {
        var list = new LinkedList<Object>();
        for (int i = 0; i < size; i++) {
            list.add(i);
        }
        return list;
    }
}

Here indeed ArrayList looses in memory comapring to LinkedList in single-item case:

arrayList:·gc.alloc.rate.norm                       1  avgt   40    80.005 ±   0.001    B/op
arrayList:·gc.alloc.rate.norm                       2  avgt   40    80.006 ±   0.001    B/op
arrayList:·gc.alloc.rate.norm                       3  avgt   40    80.007 ±   0.001    B/op
arrayList:·gc.alloc.rate.norm                       4  avgt   40    80.008 ±   0.001    B/op
arrayList:·gc.alloc.rate.norm                       5  avgt   40    80.008 ±   0.001    B/op

linkedList:·gc.alloc.rate.norm                      1  avgt   40    56.004 ±   0.001    B/op
linkedList:·gc.alloc.rate.norm                      2  avgt   40    80.005 ±   0.001    B/op
linkedList:·gc.alloc.rate.norm                      3  avgt   40   104.007 ±   0.001    B/op
linkedList:·gc.alloc.rate.norm                      4  avgt   40   128.009 ±   0.001    B/op
linkedList:·gc.alloc.rate.norm                      5  avgt   40   152.010 ±   0.001    B/op

And indeed there's at least one usecase in affected files where this is real-life scenario - JarIndex. Below on screenshot I run Spring Boot tests with Gradle:

image

However, for the same scenario one node represented with LinkedList can hold dozens of items:

image

To fix this I propose to instantiate ArrayList with initial size = 1:

@Benchmark
public Object arrayList_sized() {
  var list = new ArrayList<>(1);
  list.add(new Object());
  return list;
}

This reduces the footprint of a list with 1 element down to 48 bytes:

arrayList_sized:·gc.alloc.rate.norm                      1  avgt   40    48.004 ±   0.001    B/op

Taking this into account for the files affected we can draw the following conclusions:

  • ProxyGenerator: list is instantiated with default constructor and is likely to remain empty or hold 1 item (exception is thrown otherwise), so it's reasonable to pass 1 into constructor
  • ResourceBundle: quite a rare use-case for Norwegian language, I think we can keep default constructor. For getResource() by default list with three items is returned ("en_US", "en", ""), keep default constructor
  • URLClassPath: bound to JarIndex, list not directly instantiated
  • JarIndex: sized constructor in addToList, default one elsewhere

@stsypanov stsypanov closed this Jun 2, 2021
@stsypanov stsypanov deleted the purge-linked-list branch Jun 2, 2021
@stsypanov stsypanov restored the purge-linked-list branch Jun 2, 2021
@stsypanov stsypanov deleted the purge-linked-list branch Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment