RFR(XL): 8167108 - SMR and JavaThread Lifecycle

David Holmes david.holmes at oracle.com
Mon Nov 20 05:51:06 UTC 2017


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
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 


More information about the hotspot-gc-dev mailing list