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

8277090 : jsr166 refresh for jdk19 #8490

Closed
wants to merge 18 commits into from
Closed

Conversation

DougLea
Copy link
Contributor

@DougLea DougLea commented May 1, 2022

This is the jsr166 refresh for jdk19. See https://bugs.openjdk.java.net/browse/JDK-8285450 and https://bugs.openjdk.java.net/browse/JDK-8277090


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 reviews required, with at least 1 reviewer)
  • Change requires a CSR request to be approved

Issues

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8490

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 1, 2022

👋 Welcome back dl! 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 bot commented May 1, 2022

⚠️ @DougLea This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk
Copy link

openjdk bot commented May 1, 2022

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

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label May 1, 2022
@DougLea DougLea changed the title Jdk 8277090 JDK-8277090 jsr166 refresh for jdk19 May 1, 2022
@DougLea DougLea changed the title JDK-8277090 jsr166 refresh for jdk19 issue JDK-8277090 jsr166 refresh for jdk19 May 1, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label May 1, 2022
@DougLea DougLea changed the title issue JDK-8277090 jsr166 refresh for jdk19 issue JDK-8277090 : jsr166 refresh for jdk19 May 1, 2022
@openjdk openjdk bot removed the rfr Pull request is ready for review label May 1, 2022
@DougLea DougLea changed the title issue JDK-8277090 : jsr166 refresh for jdk19 issue 8277090 : jsr166 refresh for jdk19 May 1, 2022
@DougLea DougLea changed the title issue 8277090 : jsr166 refresh for jdk19 8277090 : jsr166 refresh for jdk19 May 1, 2022
@openjdk openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels May 1, 2022
@mlbridge
Copy link

mlbridge bot commented May 1, 2022

Webrevs

default:
throw new IllegalStateException("Task has not completed");
}
}
Copy link
Member

@forax forax May 1, 2022

Choose a reason for hiding this comment

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

I think the code will be more readable if a switch expression and the arrow syntax are used

return switch(state()) {
    case SUCCESS -> {
        @SuppressWarnings("unchecked")
        V result = (V) outcome;
        yield result;
    } 
    case FAILED -> throw new IllegalStateException("Task completed with exception");
    ...
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand how it would be more readable. I like new switch features because they extend the range of applicability of switch. But this (and the two others) are just plain vanilla enum-based switches that were handled well, and seem to have the same numbers of lines and structure either way?

Copy link
Member

Choose a reason for hiding this comment

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

It's more readable because it's now clear that the aim is to return the value of the switch.
Also a switch expression has to be exhaustive, so there is no need for a 'default'.

default:
throw new IllegalStateException("Task has not completed");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here !

Thread.yield();
s = state;
}
switch (s) {
Copy link
Member

Choose a reason for hiding this comment

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

same here !

@@ -135,7 +135,7 @@
* @since 1.5
* @author Doug Lea
*/
public interface ExecutorService extends Executor {
public interface ExecutorService extends Executor, AutoCloseable {
Copy link
Member

Choose a reason for hiding this comment

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

The class documentation should be expanded to explain that an ExecutorService can be used with a try-with-resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most AutoCloseables do not mention this in class-level javadocs. Is there a reason you think this one should?

Copy link
Member

Choose a reason for hiding this comment

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

close() is now equivalent to the method shutdownAndAwaitTermination() shown in the javadoc so i believe , replacing it with a try-with-resources is better in term of documenting how an executor service can be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mlbridge
Copy link

mlbridge bot commented May 2, 2022

Mailing list message from Doug Lea on core-libs-dev:

On 5/1/22 08:29, Doug Lea wrote:

This is the jsr166 refresh for jdk19. Seehttps://bugs.openjdk.java.net/browse/JDK-8285450 andhttps://bugs.openjdk.java.net/browse/JDK-8277090

Hopefully a harmless glitch: An initially bad merge with
(https://github.com/openjdk/jdk/commit/bba456a8dbf9027e4b015567c17a79fc7441aa08
8285676: Add missing @param <https://github.com/param> tags for type
parameters on classes) caused some unrelated commits to be added to this
PR.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label May 2, 2022
Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

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

I've been testing the ForkJoinPool update for some time and the performance improvements for the FIFO/async-mode for very impressive.

One behavior change from JDK 17/18 is that TLs are now preserved by the threads in the common pool for the no-SM case. This may be observed as "memory leak" in some environments but a help in others.

@openjdk
Copy link

openjdk bot commented May 3, 2022

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

8277090: jsr166 refresh for jdk19

Reviewed-by: alanb, psandoz

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

  • efcd3d3: 8286088: add comment to InstallAsyncExceptionHandshake destructor
  • f82dd76: 8285885: Replay compilation fails with assert(is_valid()) failed: check invoke
  • be67acd: 8285832: runtime/Thread/TooSmallStackSize.java failed "assert(k->is_initialized()) failed: need to increase java_thread_min_stack_allowed calculation"
  • 39e50c2: 8273506: java Robot API did the 'm' keypress and caused /awt/event/KeyEvent/KeyCharTest/KeyCharTest.html is timing out on macOS 12
  • 3cbf769: 8285977: Add links to IEEE 754 specification
  • 4434c7d: 8265360: several compiler/whitebox tests fail with "private compiler.whitebox.SimpleTestCaseHelper(int) must be compiled"
  • ffca23a: 8284490: Remove finalizer method in java.security.jgss
  • 0f62cb6: 8285921: serviceability/dcmd/jvmti/AttachFailed/AttachReturnError.java fails on Alpine
  • 6fcd322: 8279622: C2: miscompilation of map pattern as a vector reduction
  • af1ee1c: 8283684: IGV: speed up filter application
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/3d07b3c7f01b60ff4dc38f62407c212b48883dbf...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.

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 3, 2022
}

@Override
public Throwable exceptionNow() {
Copy link
Member

Choose a reason for hiding this comment

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

The unwrapping of CompletionExceptions should be documented

Copy link
Member

Choose a reason for hiding this comment

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

exceptionNow is specified to call get, which does not throw CompletionException:

     * @implSpec
     * The default implementation invokes {@code isDone()} to test if the task
     * has completed. If done and not cancelled, it invokes {@code get()} and
     * catches the {@code ExecutionException} to obtain the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As (almost) mentioned by others, this is legal because CompletionException is a RuntimeException, and would not be unexpected by CompletableFuture users. (The rules for when it is used have a designed-by-committee feel, because they were!)

@mlbridge
Copy link

mlbridge bot commented May 3, 2022

Mailing list message from Jason Mehrens on core-libs-dev:

Hi Doug,

In Future::exceptionNow() and Future::state() I would think we would want to catch CancellationException since the implementation of the Future is not known. Even though we pre-screen the state I would imagine there could be an implementation that prefers cancellation over normal completion.

For Future::resultNow() and FutureTask::resultNow(), is it intentional that we are not chaining just ExecutionException::getCause() / (Throwable) outcome as the cause of the IllegalStateException? Chaining that might help with debugging even if that is not how it is suppose to be used.

Jason

________________________________________
From: core-libs-dev <core-libs-dev-retn at openjdk.java.net> on behalf of Doug Lea <dl at openjdk.java.net>
Sent: Sunday, May 1, 2022 7:29 AM
To: core-libs-dev at openjdk.java.net
Subject: RFR: 8277090 : jsr166 refresh for jdk19

This is the jsr166 refresh for jdk19. See https://bugs.openjdk.java.net/browse/JDK-8285450 and https://bugs.openjdk.java.net/browse/JDK-8277090

-------------

Commit messages:
- whitespace
- sync with loom
- 8276202: LogFileOutput.invalid_file_vm asserts when being executed from a read only working directory
- 8284981: Support the vectorization of some counting-down loops in SLP
- 8284992: Fix misleading Vector API doc for LSHR operator
- 8254759: [TEST_BUG] [macosx] javax/swing/JInternalFrame/4202966/IntFrameCoord.html fails
- 8285945: [BACKOUT] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
- 8284331: Add sanity check for signal handler modification warning.
- 8285773: Replace Algorithms.eatMemory(...) with WB.fullGC() in vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
- 8284883: JVM crash: guarantee(sect->end() <= sect->limit()) failed: sanity on AVX512
- ... and 6 more: https://git.openjdk.java.net/jdk/compare/3d07b3c7...501a21e

Changes: https://git.openjdk.java.net/jdk/pull/8490/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8490&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8277090
Stats: 3750 lines in 30 files changed: 2060 ins; 656 del; 1034 mod
Patch: https://git.openjdk.java.net/jdk/pull/8490.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8490/head:pull/8490

PR: https://git.openjdk.java.net/jdk/pull/8490

@mlbridge
Copy link

mlbridge bot commented May 3, 2022

Mailing list message from Alan Bateman on core-libs-dev:

On 03/05/2022 17:14, Jason Mehrens wrote:

Hi Doug,

In Future::exceptionNow() and Future::state() I would think we would want to catch CancellationException since the implementation of the Future is not known. Even though we pre-screen the state I would imagine there could be an implementation that prefers cancellation over normal completion.

Is this a Future implementation that doesn't implement the spec
correctly? The get method shouldn't throw CancellationException if done
and not-cancelled.

For Future::resultNow() and FutureTask::resultNow(), is it intentional that we are not chaining just ExecutionException::getCause() / (Throwable) outcome as the cause of the IllegalStateException? Chaining that might help with debugging even if that is not how it is suppose to be used.

The intention is that ISE means a coding error. The usage of these
methods should be very close to the test for the state so I think it's
right as it is. If you set the cause then it be caught and used like a
CompletionException.

-Alan

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Choose a reason for hiding this comment

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

Looks good, although I cannot say i reviewed the fork join pool code as thoroughly, since it is particularly difficult, so i was most looking for consistency in the pattern of code.

boolean useSystemClassLoader, boolean isInnocuous) {
super(group, null, pool.nextWorkerThreadName(), 0L);
boolean useSystemClassLoader,
boolean clearThreadLocals) {
Copy link
Member

Choose a reason for hiding this comment

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

It's tempting to toggle the sense of last boolean argument to be preserveThreadLocals for consistency (given the multiple toggles as the value is propagated from the newly added protected constructor), but which means toggling the value at the use sites that you may not wish to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I changed param name to clearThreadLocals, which I think helps. I do need (only) one negation somewhere along these paths because internally clearing requires a set config bit.

}

@Override
public Throwable exceptionNow() {
Copy link
Member

Choose a reason for hiding this comment

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

exceptionNow is specified to call get, which does not throw CompletionException:

     * @implSpec
     * The default implementation invokes {@code isDone()} to test if the task
     * has completed. If done and not cancelled, it invokes {@code get()} and
     * catches the {@code ExecutionException} to obtain the exception.

@mlbridge
Copy link

mlbridge bot commented May 3, 2022

Mailing list message from Jason Mehrens on core-libs-dev:

Hi Alan,

Is this a Future implementation that doesn't implement the spec
correctly? The get method shouldn't throw CancellationException if done
and not-cancelled.

What you say makes sense. I should have checked this before I brought it up but CancellationException is an IllegalStateException so even if it did throw CE it is compliant with the docs.

The intention is that ISE means a coding error. The usage of these
methods should be very close to the test for the state so I think it's
right as it is. If you set the cause then it be caught and used like a
CompletionException.

Bugs sometimes cause other bugs and dropping the chain can hide that information about a root problem. I understand the reluctance to add the cause.

Thanks!

Jason

@DougLea
Copy link
Contributor Author

DougLea commented May 4, 2022

Out of caution about the extraneous commits in this PR, I will close this, and replace with a v2 momentarily.

@DougLea DougLea closed this May 4, 2022
@DougLea DougLea deleted the JDK-8277090 branch May 6, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org ready Pull request is ready to be integrated rfr Pull request is ready for review