Re: RFR: 8263561: Re-examine uses of LinkedList [v6]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - ... and 1 more: https://git.openjdk.java.net/jdk/compare/7cc1eb3e...dea42cac - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=05 Stats: 47 lines in 9 files changed: 0 ins; 2 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v5]
On Mon, 26 Jul 2021 08:27:20 GMT, Сергей Цыпанов wrote: >> After I've renamed remove branch GitHub for some reason has closed original >> https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > ># Conflicts: ># src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java > - Merge branch 'master' into purge-linked-list > - 8263561: Use sized constructor where reasonable > - 8263561: Use interface List instead of particular type where possible > - 8263561: Rename requestList -> requests > - 8263561: Re-examine uses of LinkedList I always approve of removing LinkedLists and Vectors. Using ArrayDeque in AbstractPoller seems like the right choice. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v5]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=04 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v4]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=03 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v3]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=02 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v2]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=01 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
RFR: 8263561: Re-examine uses of LinkedList
After I've renamed remove branch GitHub for some reason has closed original https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. - Commit messages: - Merge branch 'master' into 8263561 - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263561 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263561: Re-examine uses of LinkedList [v4]
> 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 Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: - all: https://git.openjdk.java.net/jdk/pull/2744/files - new: https://git.openjdk.java.net/jdk/pull/2744/files/40910fae..89160b3e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=02-03 Stats: 763526 lines in 11131 files changed: 166175 ins; 560683 del; 36668 mod Patch: https://git.openjdk.java.net/jdk/pull/2744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2744/head:pull/2744 PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList [v3]
> 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 Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8263561: Use sized constructor where reasonable - Changes: - all: https://git.openjdk.java.net/jdk/pull/2744/files - new: https://git.openjdk.java.net/jdk/pull/2744/files/158006c6..40910fae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2744/head:pull/2744 PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList [v2]
> 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 Сергей Цыпанов has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains three new commits since the last revision: - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: - all: https://git.openjdk.java.net/jdk/pull/2744/files - new: https://git.openjdk.java.net/jdk/pull/2744/files/73029fe1..158006c6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=00-01 Stats: 17 lines in 2 files changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/2744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2744/head:pull/2744 PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Mon, 24 May 2021 10:13:55 GMT, Сергей Цыпанов wrote: >> But don't people access these internal code at their own risk, as jdk may >> change these code at any time without notice? > > True, I just wonder whether it's OK to change internals when we know for sure > that this breaks 3rd party code 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Mon, 24 May 2021 09:25:08 GMT, liach wrote: >> Should we then revert the changes to `JarIndex`? > > But don't people access these internal code at their own risk, as jdk may > change these code at any time without notice? True, I just wonder whether it's OK to change internals when we know for sure that this breaks 3rd party code - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Mon, 24 May 2021 07:13:29 GMT, Сергей Цыпанов wrote: >> Just for completeness, they're using the add-exports: >> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/launchers/itw-modularjdk.args#L19 > > Should we then revert the changes to `JarIndex`? But don't people access these internal code at their own risk, as jdk may change these code at any time without notice? - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 15:12:20 GMT, Thiago Henrique Hüpner wrote: >>> 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. > > Just for completeness, they're using the add-exports: > https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/launchers/itw-modularjdk.args#L19 Should we then revert the changes to `JarIndex`? - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:18:16 GMT, Daniel Fuchs wrote: >> 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 > > src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 264: > >> 262: String jar = jarFiles[i]; >> 263: bw.write(jar + "\n"); >> 264: ArrayList jarlist = jarMap.get(jar); > > Here again, jarList could probably be declared as `List` Done - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:15:53 GMT, Daniel Fuchs wrote:
>> 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
>
> src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 155:
>
>> 153: */
>> 154: public List get(String fileName) {
>> 155: ArrayList jarFiles;
>
> This could probably be declared as:
>
>
> List jarFiles;
Done
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:22:47 GMT, Daniel Fuchs wrote: >> 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 > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 15:00:15 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:
>>
>>> 152: * @param fileName the key of the mapping
>>> 153: */
>>> 154: public List get(String fileName) {
>>
>> 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
>
>> 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-options to export jdk.internal.util.jar.
Just for completeness, they're using the add-exports:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/launchers/itw-modularjdk.args#L19
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:52:21 GMT, Thiago Henrique Hüpner wrote: > 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-options to export jdk.internal.util.jar. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов
wrote:
> 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
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:
> 152: * @param fileName the key of the mapping
> 153: */
> 154: public List get(String fileName) {
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
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов
wrote:
> 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
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.
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 155:
> 153: */
> 154: public List get(String fileName) {
> 155: ArrayList jarFiles;
This could probably be declared as:
List jarFiles;
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 264:
> 262: String jar = jarFiles[i];
> 263: bw.write(jar + "\n");
> 264: ArrayList jarlist = jarMap.get(jar);
Here again, jarList could probably be declared as `List`
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 13:13:04 GMT, Сергей Цыпанов wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > 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 Hi guys, any more comments here? Please ping me if I've missed something - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Mon, 15 Mar 2021 06:56:00 GMT, Alan Bateman wrote: >> Nothing outside of the JDK should be hacking into this private field of a >> non-exposed class, this should not be a concern here. > >> @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). @AlanBateman looks like `List.add(o, i)` is not used at all in scope of these changes. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:26:07 GMT, Alan Bateman wrote: >> 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. > > Nothing outside of the JDK should be hacking into this private field of a > non-exposed class, this should not be a concern here. > @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). - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:26:07 GMT, Alan Bateman wrote: >> 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. > > Nothing outside of the JDK should be hacking into this private field of a > non-exposed class, this should not be a concern here. @AlanBateman so is it ok to keep `ArrayLists`? - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:18:11 GMT, Сергей Цыпанов wrote: >> 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) > > 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. Nothing outside of the JDK should be hacking into this private field of a non-exposed class, this should not be a concern here. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 15:02:03 GMT, liach wrote: >> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 220: >> >>> 218: return Collections.emptyList(); >>> 219: } >>> 220: List result = new ArrayList<>(); >> >> We'd better be cautious about this replacement since its >> [caller](https://github.com/openjdk/jdk/blob/73029fe10a8a814a8c5f5221f2e667fd14a5b379/src/java.base/share/classes/java/net/URLClassLoader.java#L363) >> 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. > > 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) 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 14:58:11 GMT, Yi Yang wrote: >> 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 > > src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 220: > >> 218: return Collections.emptyList(); >> 219: } >> 220: List result = new ArrayList<>(); > > We'd better be cautious about this replacement since its > [caller](https://github.com/openjdk/jdk/blob/73029fe10a8a814a8c5f5221f2e667fd14a5b379/src/java.base/share/classes/java/net/URLClassLoader.java#L363) > 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. 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) - PR: https://git.openjdk.java.net/jdk/pull/2744
RFR: 8263561: Re-examine uses of LinkedList
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 - Commit messages: - Remove remaining usages of LinkedList in java.base Changes: https://git.openjdk.java.net/jdk/pull/2744/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2744&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263561 Stats: 40 lines in 9 files changed: 0 ins; 2 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/2744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2744/head:pull/2744 PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > 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 src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 220: > 218: return Collections.emptyList(); > 219: } > 220: List result = new ArrayList<>(); We'd better be cautious about this replacement since its [caller](https://github.com/openjdk/jdk/blob/73029fe10a8a814a8c5f5221f2e667fd14a5b379/src/java.base/share/classes/java/net/URLClassLoader.java#L363) 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 15:32:57 GMT, liach wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > 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 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. - PR: https://git.openjdk.java.net/jdk/pull/2744
