RFR(XL): 8167108 - SMR and JavaThread Lifecycle

Robin Westberg robin.westberg at oracle.com
Mon Nov 20 09:48:15 UTC 2017


Hi Dan,

Overall I must say this looks very nice, can’t wait to use it! (Also a disclaimer: not a reviewer, and have not looked at the gc or jmvti specific changes nor the tests (yet)). 

Didn’t spot any real issues, just a few small efficiency related things:

src/hotspot/share/runtime/biasedLocking.cpp

217     for (JavaThread* cur_thread = Threads::first(); cur_thread != NULL; cur_thread = cur_thread->next()) {
218       if (cur_thread == biased_thread) {
219         thread_is_alive = true;

This loop could be replaced with:

ThreadsListHandle tlh;
thread_is_alive = tlh.includes(biased_thread);


src/hotspot/share/runtime/memprofiler.cpp

55-56 grabs the Threads_lock, shouldn’t be needed anymore.


src/hotspot/share/runtime/thread.inline.hpp

198   TerminatedTypes l_terminated = (TerminatedTypes)
199       OrderAccess::load_acquire((volatile jint *) &_terminated);
200   return check_is_terminated(_terminated);

The variable used in the return statement was intended to be l_terminated I guess? 


The following are more minor suggestions / comments:

src/hotspot/share/runtime/thread.cpp

3432 // operations over all threads.  It is protected by its own Mutex
3433 // lock, which is also used in other contexts to protect thread

Should this comment perhaps be revised to mention SMR?

4745   hash_table_size--;
4746   hash_table_size |= hash_table_size >> 1;
...

This calculation is repeated around line 4922 as well, perhaps put it in a function?

4828   // If someone set a handshake on us just as we entered exit path, we simple cancel it.

Perhaps something like “.. entered the exit path, we simply cancel it.”


src/hotspot/share/runtime/thread.hpp

1179   bool on_thread_list() { return _on_thread_list; }
1180   void set_on_thread_list() { _on_thread_list = true; }
1181 
1182   // thread has called JavaThread::exit() or is terminated
1183   bool is_exiting() const;
1184   // thread is terminated (no longer on the threads list); we compare
1185   // against the two non-terminated values so that a freed JavaThread
1186   // will also be considered terminated.
1187   bool check_is_terminated(TerminatedTypes l_terminated) const {
1188     return l_terminated != _not_terminated && l_terminated != _thread_exiting;
1189   }
1190   bool is_terminated();

Use of const is inconsistent here, it’s added to 1183, should it perhaps be added to 1179 and 1190 as well then?


src/hotspot/share/runtime/vm_operations.hpp

406   DeadlockCycle* result()               { return _deadlocks; };
407   VMOp_Type type() const                { return VMOp_FindDeadlocks; }

Whitespace only change that seems spurious.

Best regards,
Robin

> On 19 Nov 2017, at 02:49, Daniel D. Daugherty <daniel.daugherty at oracle.com> 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