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

8234131: Miscellaneous changes imported from jsr166 CVS 2021-01 #1647

Closed
wants to merge 1 commit into from

Conversation

@Martin-Buchholz
Copy link
Member

@Martin-Buchholz Martin-Buchholz commented Dec 6, 2020

/cc core-libs


Progress

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

Issues

  • JDK-8234131: Miscellaneous changes imported from jsr166 CVS 2021-01
  • JDK-8257671: ThreadPoolExecutor.Discard*Policy: rejected tasks are not cancelled

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647
$ git checkout pull/1647

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 6, 2020

👋 Welcome back martin! 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.

Loading

@openjdk openjdk bot added the core-libs label Dec 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 6, 2020

@Martin-Buchholz
The core-libs label was successfully added.

Loading

@Martin-Buchholz Martin-Buchholz force-pushed the JDK-8234131 branch 2 times, most recently from 8e660fd to 390e60e Dec 6, 2020
@Martin-Buchholz
Copy link
Member Author

@Martin-Buchholz Martin-Buchholz commented Dec 6, 2020

/issue add JDK-8234131

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 6, 2020

@Martin-Buchholz This issue is referenced in the PR title - it will now be updated.

Loading

@Martin-Buchholz Martin-Buchholz force-pushed the JDK-8234131 branch 12 times, most recently from ccdc2d1 to ed43b3f Dec 8, 2020
@Martin-Buchholz Martin-Buchholz marked this pull request as ready for review Dec 8, 2020
@openjdk openjdk bot added the rfr label Dec 8, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

Webrevs

  • 03: Full (3abc843)
  • 02: Full (a96e284edc6d425aa3edf99a7ae40ff1cb829729)
  • 01: Full - Incremental (a6d85863d7a961fa51a664f35c4dd5427b03b3b3)
  • 00: Full (ed43b3fe9a2d269f9b235ca624fcaa00381b32ad)

Loading

@shipilev
Copy link
Contributor

@shipilev shipilev commented Dec 8, 2020

It would be nice to get it tested with GH Actions. "Checks" tabs should have those testing results automatically on push, but it is empty. Probably because your fork is not configured for it. Please go to https://github.com/Martin-Buchholz/jdk/actions -- and see if it says anything suspicious? Triggering the (only) workflow manually against your branch would help too.

Loading

int k = Math.min(delta, workQueue.size());
while (k-- > 0 && addWorker(null, true)) {
if (workQueue.isEmpty())
break;
}
*/
Copy link
Contributor

@AlanBateman AlanBateman Dec 8, 2020

Choose a reason for hiding this comment

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

Is this meant to be commented out?

Loading

Copy link
Member

@pavelrappo pavelrappo Dec 8, 2020

Choose a reason for hiding this comment

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

This code was also commented out in the original CVS repo; here's the diff: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.193&r2=1.194

The message for the 1.194 revision suggests we should NOT expect code changes. I've double-checked my patch, which is at least partially responsible for the 1.194 revision, and couldn't find that commenting out part.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

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

On 12/8/20 3:56 AM, Alan Bateman wrote:

1558: break;
1559: }
1560: */
Is this meant to be commented out?

Yes, but It should be marked as a possible improvement, not yet
committed. While this (prestarting) would improve performance in some
scenarios, it may also disrupt expectations and even tooling in some
existing usages, which we haven't fully checked out.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

Mailing list message from Martin Buchholz on core-libs-dev:

OK, rollback committed to CVS:

--- src/main/java/util/concurrent/ThreadPoolExecutor.java 27 Nov 2020
17:42:00 -0000 1.194
+++ src/main/java/util/concurrent/ThreadPoolExecutor.java 8 Dec 2020
20:31:54 -0000
@@ -1522,13 +1522,11 @@
// As a heuristic, prestart enough new workers (up to new
// core size) to handle the current number of tasks in
// queue, but stop if queue becomes empty while doing so.
- /*
int k = Math.min(delta, workQueue.size());
while (k-- > 0 && addWorker(null, true)) {
if (workQueue.isEmpty())
break;
}
- */
}
}

On Tue, Dec 8, 2020 at 4:04 AM Doug Lea <dl at cs.oswego.edu> wrote:

On 12/8/20 3:56 AM, Alan Bateman wrote:

1558: break;
1559: }
1560: */
Is this meant to be commented out?
Yes, but It should be marked as a possible improvement, not yet
committed. While this (prestarting) would improve performance in some
scenarios, it may also disrupt expectations and even tooling in some
existing usages, which we haven't fully checked out.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

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

8234131: Miscellaneous changes imported from jsr166 CVS 2021-01
8257671: ThreadPoolExecutor.Discard*Policy: rejected tasks are not cancelled

Reviewed-by: alanb, prappo, dl

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

  • 63e3bd7: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
  • 5cfa8c9: 8246585: ForkJoin updates
  • 6472104: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException
  • a653928: 8259512: Update --release 16 symbol information for JDK 16 build 31
  • 7e6677b: 8259446: runtime/jni/checked/TestCheckedReleaseArrayElements.java fails with stderr not empty
  • 628c546: 8258796: [test] Apply HexFormat to tests for java.security
  • 876c7fb: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys
  • 090bd3a: 8259397: ThreadsSMRSupport::print_info_on() should use try_lock_without_rank_check()
  • 10a6b0d: 8234773: Fix ThreadsSMRSupport::_bootstrap_list
  • 6f7723b: 8258792: LogCompilation: remove redundant check fixed by 8257518
  • ... and 254 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...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.

Loading

@openjdk openjdk bot added the ready label Dec 9, 2020
Copy link
Member

@pavelrappo pavelrappo left a comment

The changes to doc comments look good.

Thanks for incorporating my earlier code snippet suggestions published on concurrency-interest!

Loading

* <pre> {@code
* new RejectedExecutionHandler() {
* public void rejectedExecution(Runnable r, ThreadPoolExecutor e) {
* Future<?> dropped = e.getQueue().poll();
* if (dropped != null)
* dropped.cancel(false); // also consider logging the failure
* e.execute(r); // retry
* }}}</pre>
Copy link
Member

@pavelrappo pavelrappo Dec 9, 2020

Choose a reason for hiding this comment

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

I tried to reify that code snippet:

$ cat MySnippet.java 
import java.util.concurrent.Future;
import java.util.concurrent.RejectedExecutionHandler;
import java.util.concurrent.ThreadPoolExecutor;

public class MySnippet {
    public static void main(String[] args) {
        new RejectedExecutionHandler() {
            public void rejectedExecution(Runnable r, ThreadPoolExecutor e) {
                Future<?> dropped = e.getQueue().poll();
                if (dropped != null)
                    dropped.cancel(false); // also consider logging the failure
                e.execute(r);              // retry
            }
        };
    }
}
$ javac MySnippet.java 
MySnippet.java:9: error: incompatible types: Runnable cannot be converted to Future<?>
                Future<?> dropped = e.getQueue().poll();
                                                     ^
1 error
$ 

Separately, it seems that the if statement uses a 3-space indent which is surprising to see in the JSR 166 code base.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 9, 2020

Mailing list message from Martin Buchholz on core-libs-dev:

Thanks, Pavel!

I committed this to CVS, but am not planning to include it in this
integration:
(not sure if re-generating will invalidate my "Ready" status)
(Having to cast to Future makes this more problematic, but in typical
usages the user will know that the queue contains Futures)

--- src/main/java/util/concurrent/ThreadPoolExecutor.java 8 Dec 2020
20:33:21 -0000 1.195
+++ src/main/java/util/concurrent/ThreadPoolExecutor.java 9 Dec 2020
22:05:21 -0000 1.197
@@ -2066,10 +2066,12 @@
* <pre> {@code
* new RejectedExecutionHandler() {
* public void rejectedExecution(Runnable r, ThreadPoolExecutor e) {
- * Future<?> dropped = e.getQueue().poll();
- * if (dropped != null)
- * dropped.cancel(false); // also consider logging the failure
- * e.execute(r); // retry
+ * Runnable dropped = e.getQueue().poll();
+ * if (dropped instanceof Future<?>) {
+ * ((Future<?>)dropped).cancel(false);
+ * // also consider logging the failure
+ * }
+ * e.execute(r); // retry
* }}}</pre>
*/
public static class DiscardOldestPolicy implements
RejectedExecutionHandler {

On Wed, Dec 9, 2020 at 10:46 AM Pavel Rappo <prappo at openjdk.java.net> wrote:

Loading

@JanecekPetr
Copy link

@JanecekPetr JanecekPetr commented Dec 11, 2020

This also fixes issue JDK-8257671: ThreadPoolExecutor.Discard*Policy: rejected tasks are not cancelled, by updating the TPE.Discard*Policy JavaDocs.

I filed it days before the code went in as I did not see the JavaDoc update in JDK, sorry for my impatience and thank you for the fix. I cannot update the Jira, please link it to this PR, too.

Loading

@Martin-Buchholz
Copy link
Member Author

@Martin-Buchholz Martin-Buchholz commented Dec 14, 2020

/issue add 8257671

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 14, 2020

@Martin-Buchholz
Adding additional issue to issue list: 8257671: ThreadPoolExecutor.Discard*Policy: rejected tasks are not cancelled.

Loading

@Martin-Buchholz
Copy link
Member Author

@Martin-Buchholz Martin-Buchholz commented Dec 14, 2020

@JanecekPetr Thank you very much. I've updated JIRA and this pull request for JDK-8257671

Loading

@Martin-Buchholz Martin-Buchholz changed the title 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 8234131: Miscellaneous changes imported from jsr166 CVS 2021-01 Jan 9, 2021
@Martin-Buchholz
Copy link
Member Author

@Martin-Buchholz Martin-Buchholz commented Jan 9, 2021

/integrate

Loading

@openjdk openjdk bot closed this Jan 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 9, 2021

@Martin-Buchholz Since your change was applied there have been 264 commits pushed to the master branch:

  • 63e3bd7: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
  • 5cfa8c9: 8246585: ForkJoin updates
  • 6472104: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException
  • a653928: 8259512: Update --release 16 symbol information for JDK 16 build 31
  • 7e6677b: 8259446: runtime/jni/checked/TestCheckedReleaseArrayElements.java fails with stderr not empty
  • 628c546: 8258796: [test] Apply HexFormat to tests for java.security
  • 876c7fb: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys
  • 090bd3a: 8259397: ThreadsSMRSupport::print_info_on() should use try_lock_without_rank_check()
  • 10a6b0d: 8234773: Fix ThreadsSMRSupport::_bootstrap_list
  • 6f7723b: 8258792: LogCompilation: remove redundant check fixed by 8257518
  • ... and 254 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...master

Your commit was automatically rebased without conflicts.

Pushed as commit 270014a.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants