8274715: Implement forEach in Collections.CopiesList

[stsypanov]

Originally was proposed by Zheka Kozlov here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-December/057192.html

Just a tiny optimization: we can use for-i loop instead of Iterable.forEach() which is relying on iterator.

Simple benchmark demonstrates slight improvement:

@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class NCopiesBenchmarks {
  @Param({"10", "50", "100"})
  int size;

  private List<Object> list;

  @Setup
  public void prepare() {
    list = Collections.nCopies(size, new Object());
  }

  @Benchmark
  public void forEach(Blackhole bh) {
    list.forEach(bh::consume);
  }
}

before

Benchmark                                      (size)  Mode  Cnt    Score    Error   Units
NCopiesBenchmarks.forEach                          10  avgt   50   40.737 ±  1.854   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate           10  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm      10  avgt   50   ≈ 10⁻⁴             B/op
NCopiesBenchmarks.forEach:·gc.count                10  avgt   50      ≈ 0           counts
NCopiesBenchmarks.forEach                          50  avgt   50  213.324 ±  3.784   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate           50  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm      50  avgt   50   ≈ 10⁻³             B/op
NCopiesBenchmarks.forEach:·gc.count                50  avgt   50      ≈ 0           counts
NCopiesBenchmarks.forEach                         100  avgt   50  443.171 ± 17.919   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate          100  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm     100  avgt   50    0.001 ±  0.001    B/op
NCopiesBenchmarks.forEach:·gc.count               100  avgt   50      ≈ 0           counts

after

Benchmark                                      (size)  Mode  Cnt    Score    Error   Units
NCopiesBenchmarks.forEach                          10  avgt   50   36.838 ±  0.065   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate           10  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm      10  avgt   50   ≈ 10⁻⁴             B/op
NCopiesBenchmarks.forEach:·gc.count                10  avgt   50      ≈ 0           counts
NCopiesBenchmarks.forEach                          50  avgt   50  191.173 ±  0.570   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate           50  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm      50  avgt   50   ≈ 10⁻⁴             B/op
NCopiesBenchmarks.forEach:·gc.count                50  avgt   50      ≈ 0           counts
NCopiesBenchmarks.forEach                         100  avgt   50  376.675 ±  2.476   ns/op
NCopiesBenchmarks.forEach:·gc.alloc.rate          100  avgt   50    0.001 ±  0.001  MB/sec
NCopiesBenchmarks.forEach:·gc.alloc.rate.norm     100  avgt   50    0.001 ±  0.001    B/op
NCopiesBenchmarks.forEach:·gc.count               100  avgt   50      ≈ 0           counts

---

Progress

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

Issue

• JDK-8274715: Implement forEach in Collections.CopiesList

Reviewers

• @liach (no known github.com user name / role) ⚠️ Review applies to 2303aa7
• Martin Buchholz (@Martin-Buchholz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2524

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

Using diff file

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

[bridgekeeper]

👋 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]

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

[bridgekeeper]

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

[bridgekeeper]

@stsypanov This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[stsypanov]

/open

[openjdk]

@stsypanov @HostUserDetails{id=10835776, username='stsypanov', fullName='null'} this pull request is now open

[mlbridge]

Webrevs

• 02: Full - Incremental (7655be8f)
• 01: Full - Incremental (18f5d589)
• 00: Full (2303aa72)

[Martin-Buchholz]

Core collection classes should have optimized versions of forEach, so this is a good change in principle. Although CopiesList.forEach is unlikely to be performance critical.

I implemented many similar optimizations for core collection classes in past years.
Many of them are benchmarked in test/jdk/java/util/Collection/IteratorMicroBenchmark.java
That was written pre-JMH.
I see a JMH benchmark was written, but it is not part of the commit.

There are a number of unrelated changes in this commit that look like they were suggested by a lint-like tool. Such changes are good, but they belong in a separate cleanup commit applied across large portions of the jdk sources.

I would not use "var" here - more readable with concrete types.

Similarly, I prefer not using diamond for
return new Enumeration() {

[stsypanov]

@Martin-Buchholz thanks for review! I've reverted irrelevant changes and added the benchmark.

[Martin-Buchholz]

Thanks - TIL about Blackhole::consume .

All Java source files should end with exactly one newline. Configure your editor to make it so.

[stsypanov]

Done

[Martin-Buchholz]

Looks good!

[openjdk]

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout ncopies
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

[openjdk]

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

8274715: Implement forEach in Collections.CopiesList

Reviewed-by: martin

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

• 83b2219: 8273612: Fix for JDK-8272873 causes timeout in running some tests with -Xcomp
• d34ec6c: 8274793: Suppress warnings on non-serializable non-transient instance fields in sun.net
• 332f067: 8274729: Define Position.NOPOS == Diagnostic.NOPOS
• 1e75203: 8274656: Remove default_checksum and safe_checksum_type from krb5.conf
• 03d3c03: 8273670: Remove weak etypes from default krb5 etype list
• c391e59: 8274244: ReportOnImportedModuleAnnotation.java fails on rerun
• 3789065: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code
• e1f0df0: 8267853: Remove unused styles from stylesheet
• 4e3948f: 8274744: TestSnippetTag test fails after recent integration
• 92b64a2: 8273745: VerifyLocale.java occasionally times out
• ... and 6 more: https://git.openjdk.java.net/jdk/compare/a914ee72167f642b76b1c1cdddd7ff0698d061cc...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@Martin-Buchholz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[stsypanov]

/integrate

[openjdk]

@stsypanov
Your change (at version 7655be8) is now ready to be sponsored by a Committer.

[Martin-Buchholz]

I've never sponsored a github change and https://openjdk.java.net/sponsor/ is unhelpfully stale.
Oh wait ... I now see
https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor

/sponsor

[openjdk]

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

• d4e8712: 8274797: ProblemList resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java on macosx-x64
• 4726267: 8274642: jdk/jshell/CommandCompletionTest.java fails with NoSuchElementException after JDK-8271287
• 83b2219: 8273612: Fix for JDK-8272873 causes timeout in running some tests with -Xcomp
• d34ec6c: 8274793: Suppress warnings on non-serializable non-transient instance fields in sun.net
• 332f067: 8274729: Define Position.NOPOS == Diagnostic.NOPOS
• 1e75203: 8274656: Remove default_checksum and safe_checksum_type from krb5.conf
• 03d3c03: 8273670: Remove weak etypes from default krb5 etype list
• c391e59: 8274244: ReportOnImportedModuleAnnotation.java fails on rerun
• 3789065: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code
• e1f0df0: 8267853: Remove unused styles from stylesheet
• ... and 8 more: https://git.openjdk.java.net/jdk/compare/a914ee72167f642b76b1c1cdddd7ff0698d061cc...master

Your commit was automatically rebased without conflicts.

[openjdk]

@Martin-Buchholz @stsypanov Pushed as commit df7b0c7.

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