Hi Dan,
Figured I'd better cast an eye over this again before it gets pushed :)
Only one thing (repeated many times) jumped out at me and apologies if
this has already been debated back and forth. I missed the exchange
where the for loop iteration was rewritten into this unusual form:
for (JavaThreadIteratorWithHandle jtiwh; JavaThread *jt = jtiwh.next(); ) {
On first reading I expect most people would mistake the control
expression for the iteration-expression due to the next(). I didn't even
know the control expression could introduce a local variable! I find
this really hard to read stylistically and far too cute/clever for its
own good. It also violates the style rules regarding implicit boolean
checks.
I know Stefan proposed this as he did not like the alternate (in a few
places):
+ {
+ ThreadsListHandle tlh;
+ JavaThreadIterator jti(tlh.list());
+ for (JavaThread* t = jti.first(); t != NULL; t = jti.next()) {
...
+ }
but it seems to me the iterator code has evolved since then and only a
couple of places need a distinct TLH separate from the actual iterator
object. So I'm more in favour of the more classic for-loop, with the
iterator declared before the loop. Or even convert to while loops, as
this iterator seems more suited to that.
Other than that just a couple of comments on variable type choices, and
a few typos spotted below.
Thanks,
David
---
src/hotspot/share/runtime/globals.hpp
2476 diagnostic(bool, EnableThreadSMRExtraValidityChecks, true,
\
2477 "Enable Thread SMR extra validity checks")
\
2478
\
2479 diagnostic(bool, EnableThreadSMRStatistics, true,
\
2480 "Enable Thread SMR Statistics")
\
Indent of strings is off by 3 spaces.
---
src/hotspot/share/runtime/handshake.cpp
140 // There is assumption in code that Threads_lock should be lock
200 // There is assumption in code that Threads_lock should be lock
lock -> locked
146 // handshake_process_by_vmthread will check the semaphore
for us again
Needs period at end.
148 // should be okey since the new thread will not have an
operation.
okey -> okay
151 // We can't warn here is since the thread do
cancel_handshake after it have been removed
I think:
// We can't warn here since the thread does cancel_handshake after it
has been removed
152 // from ThreadsList. So we should just keep looping here
until while below return negative.
from -> from the
Needs period at end.
204 // A new thread on the ThreadsList will not have an
operation.
Change period to comma, and ...
205 // Hence is skipped in handshake_process_by_vmthread.
Hence is -> hence it is
---
src/hotspot/share/runtime/thread.cpp
1482 // dcubed - Looks like the Threads_lock is for stable access
1483 // to _jvmci_old_thread_counters and _jvmci_counters.
Does this comment need revisiting?
3451 volatile jint ...
Why are you using jint for field types here? (Surprised Coleen hasn't
spotted them! ;-) ).
3456 long Threads::_smr_java_thread_list_alloc_cnt = 1;
3457 long Threads::_smr_java_thread_list_free_cnt = 0;
long? If you really want 64-bit counters here you won't get them on
Windows with that declaration. If you really want variable sized
counters I suggest uintptr_t; otherwise uint64_t.
----
On 19/11/2017 11:49 AM, Daniel D. Daugherty wrote:
Greetings,
Testing of the last round of changes revealed a hang in one of the new
TLH tests. Robbin has fixed the hang, updated the existing TLH test, and
added another TLH test for good measure.
Here is the updated full webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-09-full/
Here is the updated delta webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-09-delta/
Dan ran the bits thru the usual Mach5 tier[1-5] testing and there are
no unexpected failures.
We welcome comments, suggestions and feedback.
Dan, Erik and Robbin
On 11/15/17 3:32 PM, Daniel D. Daugherty wrote:
Greetings,
Robbin rebased the project last night/this morning to merge with Thread
Local Handshakes (TLH) and also picked up additional changesets up thru:
Changeset: fa736014cf28
Author: cjplummer
Date: 2017-11-14 18:08 -0800
URL:http://hg.openjdk.java.net/jdk/hs/rev/fa736014cf28
8191049: Add alternate version of pns() that is callable from within
hotspot source
Summary: added pns2() to debug.cpp
Reviewed-by: stuefe, gthornbr
This is the first time we've rebased the project to bits that are this
fresh (< 12 hours old at rebase time). We've done this because we think
we're done with this project and are in the final review-change-retest
cycle(s)... In other words, we're not planning on making any more major
changes for this project... :-)
*** Time for code reviewers to chime in on this thread! ***
Here is the updated full webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-08-full/
Here's is the delta webrev from the 2017.11.10 rebase:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-08-delta/
Dan has submitted the bits for the usual Mach5 tier[1-5] testing
(and the new baseline also)...
We're expecting this round to be a little noisier than usual because
we did not rebase on a PIT snapshot so the new baseline has not been
through Jesper's usual care-and-feeding of integration_blockers, etc.
We welcome comments, suggestions and feedback.
Dan, Erik and Robbin
On 11/14/17 4:48 PM, Daniel D. Daugherty wrote:
Greetings,
I rebased the project to the 2017.11.10 jdk/hs PIT snapshot.
(Yes, we're playing chase-the-repo...)
Here is the updated full webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-07-full/
Unlike the previous rebase, there were no changes required to the
open code to get the rebased bits to build so there is no delta
webrev.
These bits have been run through JPRT and I've submitted the usual
Mach5 tier[1-5] test run...
We welcome comments, suggestions and feedback.
Dan, Erik and Robbin
On 11/13/17 12:30 PM, Daniel D. Daugherty wrote:
Greetings,
I rebased the project to the 2017.10.26 jdk10/hs PIT snapshot.
Here are the updated webrevs:
Here's the mq comment for the change:
Rebase to 2017.10.25 PIT snapshot.
Here is the full webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-06-full/
And here is the delta webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-06-delta/
I ran the above bits throught Mach5 tier[1-5] testing over the holiday
weekend. Didn't see any issues in a quick look. Going to take a closer
look today.
We welcome comments, suggestions and feedback.
Dan, Erik and Robbin
On 11/8/17 1:05 PM, Daniel D. Daugherty wrote:
Greetings,
Resolving one of the code review comments (from both Stefan K and
Coleen)
on jdk10-04-full required quite a few changes so it is being done as a
standalone patch and corresponding webrevs:
Here's the mq comment for the change:
stefank, coleenp CR - refactor most JavaThreadIterator usage to use
JavaThreadIteratorWithHandle.
Here is the full webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-05-full/
And here is the delta webrev:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-05-delta/
We welcome comments, suggestions and feedback.
Dan, Erik and Robbin
On 10/9/17 3:41 PM, Daniel D. Daugherty wrote:
Greetings,
We have a (eXtra Large) fix for the following bug:
8167108 inconsistent handling of SR_lock can lead to crashes
https://bugs.openjdk.java.net/browse/JDK-8167108
This fix adds a Safe Memory Reclamation (SMR) mechanism based on
Hazard Pointers to manage JavaThread lifecycle.
Here's a PDF for the internal wiki that we've been using to describe
and track the work on this project:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/SMR_and_JavaThread_Lifecycle-JDK10-04.pdf
Dan has noticed that the indenting is wrong in some of the code
quotes
in the PDF that are not present in the internal wiki. We don't have a
solution for that problem yet.
Here's the webrev for current JDK10 version of this fix:
http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-04-full
This fix has been run through many rounds of JPRT and Mach5 tier[2-5]
testing, additional stress testing on Dan's Solaris X64 server, and
additional testing on Erik and Robbin's machines.
We welcome comments, suggestions and feedback.
Daniel Daugherty
Erik Osterlund
Robbin Ehn
