8303266: Prefer ArrayList to LinkedList in JImageTask

[turbanoff]

LinkedList is used as a field jdk.tools.jimage.JImageTask.OptionsValues#jimages
It's created, filled (with add) and then iterated. No removes from the head or something like this. ArrayList should be preferred as more efficient and widely used (more chances for JIT) collection.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8303266: Prefer ArrayList to LinkedList in JImageTask

Reviewers

• Jim Laskey (@JimLaskey - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12760/head:pull/12760
$ git checkout pull/12760

Update a local copy of the PR:
$ git checkout pull/12760
$ git pull https://git.openjdk.org/jdk pull/12760/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12760

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12760.diff

[bridgekeeper]

👋 Welcome back aturbanov! 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]

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

• core-libs

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

[mlbridge]

Webrevs

• 00: Full (30873840)

[JimLaskey]

LGTM. Not sure why he used that but may have started with a linked list hashmap to track duplicate files and the dropped down to linked list. At any rate, should be fine.

[openjdk]

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

8303266: Prefer ArrayList to LinkedList in JImageTask

Reviewed-by: jlaskey

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

• 784f7b1: 8293667: Align jlink's --compress option with jmod's --compress option
• 54603aa: 8303208: JFR: 'jfr print' displays incorrect timestamps
• 4c169d2: 8303253: Remove unnecessary calls to super() in java.time value based classes
• b527edd: 8292914: Lambda proxies have unstable names
• 42330d2: 8029370: (fc) FileChannel javadoc not clear for cases where position == size
• a253b46: 8301119: Support for GB18030-2022
• 55e6bb6: 8302685: Some javac unit tests aren't reliably closing open files
• f5a1276: 8262895: [macos_aarch64] runtime/CompressedOops/CompressedClassPointers.java fails with 'Narrow klass base: 0x0000000000000000' missing from stdout/stderr
• 2fe4e5f: 8303169: Remove Windows specific workaround from libdt

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[RogerRiggs]

IMHO, there is not enough value in this change to continue the review. It should be closed without integrating.

[turbanoff]

@RogerRiggs
I believe it's common knowledge that LinkedList is not a best collection nowadays.
OpenJDK source code is a reference implementation, and people look at its code.
And when people see that OpenJDK guys use LinkedList (As I can see it's still have 234 usages in OpenJDK), it's confusing and creates questions.
I believe cleaning OpenJDK repository from outdated usages could speed up LinkedList obsoleteness.

[stuart-marks]

I guess I should weigh in because I'm mentioned here by tweet.

The main point of "stop using LinkedList" (from the tweet) is in new code. If you're writing new code and get bogged down thinking, "Hm, should I use a LinkedList or ArrayList here?" then you should probably just use ArrayList and move on. There are few use cases where LinkedList is preferable to ArrayList, and most people won't ever see them.

That doesn't justify replacing existing usages of ArrayList with LinkedList. The proximate question to ask here is, "Does this change improve the JDK?" I don't see any such justification here. Functionally the replacement is intended to be the same, so this doesn't fix any actual bug. The performance is supposedly better, but it's unclear whether this actually matters in this case. However, there are risks and costs associated with such changes, including the possibility of unforeseen behavioral changes that result in test failures, and the cost of reviewer time.

The only benefit mentioned is that people look at the JDK and see LinkedList being used so they think it's OK to use LinkedList in their own code. This is quite tenuous. If you want people to stop using LinkedList, it will be much more effective to get to say "use ArrayList instead of LinkedList" instead of trying to remove uses of it from the JDK.

[RogerRiggs]

I should have expounded on the rationale for making only changes that are worth the time of the author and the reviewers.
Stuart filled more of the reasons I was thinking in my terse comment.
For this PR, go ahead and integrate, since we've already spent the budget for reviewing it.
In choosing how you spend your time, make your efforts and the efforts of the reviewers count for something.

[stuart-marks]

For this PR, go ahead and integrate, since we've already spent the budget for reviewing it.

I'm ok with this, as Roger said, since we've all already looked at it.

I'm not really interested in seeing 234 followup changes removing LinkedList from all over the JDK though.

[turbanoff]

In choosing how you spend your time, make your efforts and the efforts of the reviewers count for something.

It's one of the reasons, why I don't create PRs with massive replacements. I carefully choose places in the code, where I think replacement is 100% safe.
In this case we have a final field which has only 3 simple usages. Reviewing such small PR shouldn't take more than like 15 minutes.

If you want people to stop using LinkedList, it will be much more effective to get to say "use ArrayList instead of LinkedList"

Not sure I agree with this one. Even presence issues/PRs like this in JDK, could be used as an argument between developers, who need to choose what's better.

[stuart-marks]

Sorry, there was a markup error in my earlier comment. What I meant it to say was this:

« If you want people to stop using LinkedList, it will be much more effective to get your favorite tutorial site to say "use ArrayList instead of LinkedList" instead of trying to remove uses of it from the JDK. »

(I had put the italicized text in angle brackets, which I guess were interpreted as malformed markup and so the text was simply dropped.)

I might as well name names here. If you do a web search for "java LinkedList" you will get hits on the usual top tutorial sites. One of them is this:

https://www.w3schools.com/java/java_linkedlist.asp

The whole discussion of "ArrayList vs. LinkedList" here is incredibly superficial. The summary is this:

Use an ArrayList for storing and accessing data, and LinkedList to manipulate data.

This sort of statement is a lot more influential in getting Java developers to use LinkedList than anything the JDK does. Orders of magnitude more Java developers look at these tutorial sites than look at obscure corners of the JDK source code. If you want people to stop using LinkedList, it would be better to try to get those tutorial sites changed than to try to eradicate LinkedList from the JDK.

[turbanoff]

/integrate

[openjdk]

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

• 5f153e0: 8302780: Add support for vectorized arraycopy GC barriers
• d00a767: 8295737: macOS: Print content cut off when width > height with portrait orientation
• 15c76e4: 8301254: UNIX sun/font coding does not detect SuSE in openSUSE Leap distribution
• 3eff1a0: 8303630: Move nsk/jvmti/AttachOnDemand/attach002a/TestDescription.java back to general problem list
• 10d6a8e: 8299518: HotSpotVirtualMachine shared code across different platforms
• 148900c: 8303562: Remove obsolete comments in os::pd_attempt_reserve_memory_at
• 1bb39a9: 8302027: Port fdlibm trig functions (sin, cos, tan) to Java
• 9fdbf3c: 8303588: [JVMCI] make JVMCI source directories conform with standard layout
• 629a905: 8303242: ThreadMXBean issues with virtual threads
• 5b2e2e4: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
• ... and 81 more: https://git.openjdk.org/jdk/compare/f2b03f9a2c0fca853211e41a1ddf46195dd56698...master

Your commit was automatically rebased without conflicts.

[openjdk]

@turbanoff Pushed as commit fa1cebe.

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