8373118: Test java/lang/Thread/virtual/Starvation.java timed out #28797
Conversation
|
👋 Welcome back dl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| task = (fifo != 0) ? localPoll() : localPop(); | ||
| final int topLevelExec(ForkJoinTask<?> task, WorkQueue q, | ||
| int fifo, int qbase) { | ||
| int stolen = 1; |
There was a problem hiding this comment.
@DougLea It does seem a bit weird that stolen starts as 1 even if "task == null" (i.e. this method would return 1 for an invocation where task or q is null, which doesn't sounds right?)
There was a problem hiding this comment.
On the other hand, t is never null, and q is never null, so I'll just suggest a small comment below.
There was a problem hiding this comment.
Or less oddly, now prefaced with
if (task == null || q == null)
return 0; // currently impossible
I also added some javadoc and comments and cosmetics
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
| if (q.base != qbase || qa == null || (qcap = qa.length) <= 0 || | ||
| (t = (ForkJoinTask<?>)U.getReferenceAcquire( | ||
| qa, qk = slotOffset((qcap - 1) & qbase))) == null || | ||
| q.base != qbase || |
There was a problem hiding this comment.
Is this (repeated) check intended? (i.e. are we worried about q.base drift between polling the array?)
There was a problem hiding this comment.
Yes, needed (everywhere) to avoid bad CASes in rare cases (like array wraparound).
| if (a != null && k < a.length && k >= 0 && a[k] == null) | ||
| break; | ||
| if (c == (c = ctl) && c == (c = compareAndExchangeCtl(c, nc))) { | ||
| if (c == (c = compareAndExchangeCtl(c, nc))) { |
There was a problem hiding this comment.
I think the risk here is that this introduces more cache-coordination contention, esp. on multisocket aarch64.
| if (v == null) | ||
| createWorker(); | ||
| else { | ||
| v.phase = sp; | ||
| if (v.parking != 0) | ||
| U.unpark(v.owner); | ||
| } | ||
| break; | ||
| } | ||
| if (q != null && q.base - qbase > 0) | ||
| break; |
There was a problem hiding this comment.
Crazy idea: Only create new workers on successful CAS if no queue-drift has occurred, but always prioritize to wake up sleeping workers regardless?
| if (v != null) { | |
| v.phase = sp; | |
| if (v.parking != 0) | |
| U.unpark(v.owner); | |
| } else if (q == null || q.base - qbase == 0) | |
| createWorker(); | |
| break; | |
| } | |
| if (q != null && q.base - qbase > 0) | |
| break; |
There was a problem hiding this comment.
Narrator: Turns out it was crazy, as it requires a back-out of nc. Disregard the comment above :)
| int pc = parallelism; | ||
| for (long c = ctl;;) { | ||
| WorkQueue[] qs = queues; | ||
| long ac = (c + RC_UNIT) & RC_MASK, nc; | ||
| int sp = (int)c, i = sp & SMASK; | ||
| if ((short)(c >>> RC_SHIFT) >= pc) |
There was a problem hiding this comment.
I'm wondering if it might make sense to re-read parallelism on each new loop, since if we have a parallelism resize during this loop we want to exit early?
| for (long c = ctl;;) { | |
| WorkQueue[] qs = queues; | |
| long ac = (c + RC_UNIT) & RC_MASK, nc; | |
| int sp = (int)c, i = sp & SMASK, pc; | |
| if ((short)(c >>> RC_SHIFT) >= (pc = parallelism)) |
There was a problem hiding this comment.
Maybe but it should also be rare to adjust parallelism, and setParallelism changed recently to signal.
| int size = s - base + 1, m; | ||
| if (((a != null && a.length > size) || (a = growArray(a, s)) != null) && |
There was a problem hiding this comment.
Instruction ordering reason to assign the size ahead of the if? 🤔
int m;
if (((a != null && a.length > (s - base + 1)) || (a = growArray(a, s)) != null) &&
There was a problem hiding this comment.
(Now reworked a little...) Well, mainly trying to simply the conditional while avoiding possibly-uninitialized errors
| } | ||
| return newArray; | ||
| top = s; // back out | ||
| if ((phase & 1) == 0) // unlock if external |
There was a problem hiding this comment.
I presume the read of phase is very much intentional here (otherwise passing in boolean internal as a param would make sense), so it might be worth adding the reason for reading phase on this line? 🤔
There was a problem hiding this comment.
Yes. The main reason is that I had needed growArray in bulk-steal exploration (but not committed) and needed version without argument). But then noticed that there's no reason to have arg for a case that almost never happens.
There was a problem hiding this comment.
Makes sense, thanks for confirming 👍 :)
|
The change to relax orderings in push means that java/lang/Thread/virtual/Starvation.java now failing very frequently, on both x64 and aarch64. So this may tell us something. |
3 participants
Changes signal filtering to avoid possible starvation
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28797/head:pull/28797$ git checkout pull/28797Update a local copy of the PR:
$ git checkout pull/28797$ git pull https://git.openjdk.org/jdk.git pull/28797/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28797View PR using the GUI difftool:
$ git pr show -t 28797Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28797.diff
Using Webrev
Link to Webrev Comment