Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v4]
On Mon, 14 Dec 2020 00:40:08 GMT, Martin Buchholz wrote: >> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 > > Martin Buchholz has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8234131 Marked as reviewed by dl (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
On Fri, 11 Dec 2020 15:07:45 GMT, Petr Janeček wrote: >> The changes to doc comments look good. >> >> Thanks for incorporating my earlier code snippet suggestions published on >> [concurrency-interest](http://cs.oswego.edu/pipermail/concurrency-interest/2020-November/017264.html)! > > This also fixes issue > [JDK-8257671](https://bugs.openjdk.java.net/browse/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. @JanecekPetr Thank you very much. I've updated JIRA and this pull request for JDK-8257671 - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v4]
> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 Martin Buchholz has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: JDK-8234131 - Changes: https://git.openjdk.java.net/jdk/pull/1647/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1647=03 Stats: 314 lines in 41 files changed: 107 ins; 41 del; 166 mod Patch: https://git.openjdk.java.net/jdk/pull/1647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647 PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
On Wed, 9 Dec 2020 18:41:02 GMT, Pavel Rappo wrote: >> Martin Buchholz has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains one commit: >> >> JDK-8234131 > > The changes to doc comments look good. > > Thanks for incorporating my earlier code snippet suggestions published on > [concurrency-interest](http://cs.oswego.edu/pipermail/concurrency-interest/2020-November/017264.html)! This also fixes issue [JDK-8257671](https://bugs.openjdk.java.net/browse/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. - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
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 - 1.195
+++ src/main/java/util/concurrent/ThreadPoolExecutor.java 9 Dec 2020
22:05:21 - 1.197
@@ -2066,10 +2066,12 @@
* {@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
* }}}
*/
public static class DiscardOldestPolicy implements
RejectedExecutionHandler {
On Wed, Dec 9, 2020 at 10:46 AM Pavel Rappo wrote:
> On Tue, 8 Dec 2020 21:15:48 GMT, Martin Buchholz
> wrote:
>
> >> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12
> >
> > Martin Buchholz has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains one commit:
> >
> > JDK-8234131
>
> The changes to doc comments look good.
>
> Thanks for incorporating my earlier code snippet suggestions published on
> [concurrency-interest](
> http://cs.oswego.edu/pipermail/concurrency-interest/2020-November/017264.html
> )!
>
> src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java
> line 2102:
>
> > 2100: *dropped.cancel(false); // also consider logging the
> failure
> > 2101: * e.execute(r); // retry
> > 2102: * }}}
>
> 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.
>
> -
>
> Marked as reviewed by prappo (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/1647
>
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
On Tue, 8 Dec 2020 21:15:48 GMT, Martin Buchholz wrote:
>> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12
>
> Martin Buchholz has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains one commit:
>
> JDK-8234131
The changes to doc comments look good.
Thanks for incorporating my earlier code snippet suggestions published on
[concurrency-interest](http://cs.oswego.edu/pipermail/concurrency-interest/2020-November/017264.html)!
src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java line
2102:
> 2100: *dropped.cancel(false); // also consider logging the
> failure
> 2101: * e.execute(r); // retry
> 2102: * }}}
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.
-
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
On Tue, 8 Dec 2020 21:15:48 GMT, Martin Buchholz wrote: >> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 > > Martin Buchholz has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8234131 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 Martin Buchholz has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: JDK-8234131 - Changes: https://git.openjdk.java.net/jdk/pull/1647/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1647=02 Stats: 312 lines in 41 files changed: 105 ins; 41 del; 166 mod Patch: https://git.openjdk.java.net/jdk/pull/1647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647 PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
OK, rollback committed to CVS:
--- src/main/java/util/concurrent/ThreadPoolExecutor.java 27 Nov 2020
17:42:00 - 1.194
+++ src/main/java/util/concurrent/ThreadPoolExecutor.java 8 Dec 2020
20:31:54 -
@@ -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 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.
>
>
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
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.
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
On Tue, 8 Dec 2020 08:53:38 GMT, Alan Bateman wrote: >> Martin Buchholz 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. > > src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java line > 1560: > >> 1558: break; >> 1559: } >> 1560: */ > > Is this meant to be commented out? 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=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. - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
On Tue, 8 Dec 2020 06:11:27 GMT, Martin Buchholz wrote: >> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 > > Martin Buchholz 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. src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java line 1560: > 1558: break; > 1559: } > 1560: */ Is this meant to be commented out? - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12
On Sun, 6 Dec 2020 02:57:03 GMT, Martin Buchholz wrote: > 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 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. - PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 Martin Buchholz 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 one new commit since the last revision: JDK-8234131 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1647/files - new: https://git.openjdk.java.net/jdk/pull/1647/files/ed43b3fe..a6d85863 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1647=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1647=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647 PR: https://git.openjdk.java.net/jdk/pull/1647
RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12
8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 - Commit messages: - JDK-8234131 Changes: https://git.openjdk.java.net/jdk/pull/1647/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1647=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8234131 Stats: 314 lines in 41 files changed: 107 ins; 41 del; 166 mod Patch: https://git.openjdk.java.net/jdk/pull/1647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647 PR: https://git.openjdk.java.net/jdk/pull/1647
