RFR(XL): 8167108 - SMR and JavaThread Lifecycle

David Holmes david.holmes at oracle.com
Wed Nov 22 04:26:39 UTC 2017 

Hi Dan,

On 22/11/2017 10:12 AM, Daniel D. Daugherty wrote:
> Greetings,
> 
> *** We are wrapping up code review on this project so it is time ***
> *** for the various code reviewers to chime in one last time. ***
> 
> In this latest round, we had three different reviewers chime in so we're
> doing delta webrevs for each of those resolutions:
> 
> David H's resolutions:
> 
> http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-10.09,10.10.0-delta/ 
> 
> 
>    mq comment for dholmes_CR:
>      - Fix indents, trailing spaces and various typos.
>      - Add descriptions for the '_cnt', '_max' and '_times" fields,
>        add impl notes to document the type choices.

src/hotspot/share/runtime/thread.cpp

3466 // range. Unsigned 64-bit would be more future proof, but 64-bit 
atomic inc
3467 // isn't available everywhere (or is it?).

64-bit atomics should be available on all currently supported platforms 
(the last time this was an issue was for PPC32 - and the atomic 
templates should have filled in any previous holes in the allowed 
types). But if it's always 64-bit then you'd have to use Atomic::load to 
allow for 32-bit platforms.

These counts etc are all just for statistics so we aren't really 
concerned about eventual overflows in long running VMs - right?

---

                                        // # of parallel threads in 
_smr_delete_lock->wait().
   static uint                  _smr_delete_lock_wait_cnt;
                                        // Max # of parallel threads in 
_smr_delete_lock->wait().


why are the comments indented like that? I thought this is what Coleen 
previously commented on. Please just start the comments in the first 
column - no need for any indent. Thanks.

>    src/hotspot/share/runtime/globals.hpp change is white-space only so it
>    is only visible via the file's patch link.
> 
> 
> Robin W's resolutions:
> 
> http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-10.10.0,10.10.1-delta/ 
> 
> 
>    mq comment for robinw_CR:
>      - Fix some inefficient code, update some comments, fix some indents,
>        and add some 'const' specifiers.
> 
>    src/hotspot/share/runtime/vm_operations.hpp change is white-space 
> only so
>    it is only visible via the file's patch link.

All looks fine to me. Some good catches there from Robin!

> Coleen's resolutions:
> 
> http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-10.10.1,10.10.2-delta/ 
> 
> 
>    mq comment for coleenp_CR:
>      - Change ThreadsList::_threads from 'mtGC' -> 'mtThread',
>      - add header comment to threadSMR.hpp file,
>      - cleanup JavaThreadIteratorWithHandle ctr,
>      - make ErrorHandling more efficient.

src/hotspot/share/runtime/threadSMR.hpp

+ // Thread Safe Memory Reclaimation (Thread-SMR) support.

Only one 'i' in Reclamation :)

Thanks,
David
------

> 
> Some folks prefer to see all of the delta webrevs together so here is
> a delta webrev relative to jdk10-09-full:
> 
> http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-10.09,10.10.2-delta/ 
> 
> 
>    src/hotspot/share/runtime/globals.hpp and
>    src/hotspot/share/runtime/vm_operations.hpp changes are white-space only
>    so they are only visible via the file's patch link.
> 
> 
> And, of course, some folks prefer to see everything all at once so here
> is the latest full webrev:
> 
> http://cr.openjdk.java.net/~dcubed/8167108-webrev/jdk10-10.10-full/
> 
> 
> Dan has submitted the bits for the usual Mach5 tier[1-5] testing. The
> prelim Mach5 tier1 (JPRT equivalent) on these bits passed just fine...
> 
> The project is currently baselined on Jesper's 2017-11-17 PIT snapshot
> so there will be some catching up to do before integration...
> 
> We welcome comments, suggestions and feedback.
> 
> Dan, Erik and Robbin
> 
> 
> On 11/18/17 8:49 PM, 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 serviceability-dev mailing list