RFR(XL): 8167108 - SMR and JavaThread Lifecycle
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Nov 22 13:09:16 UTC 2017
Adding back the other OpenJDK aliases...
On 11/22/17 8:01 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 11/22/17 7:51 AM, Daniel D. Daugherty wrote:
>> On 11/21/17 11:26 PM, David Holmes wrote:
>>> 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).
>>
>> Hmmm... I ran into this when I merged the project with the atomic
>> templates. I got a JPRT build failure on one of the ARM platforms...
>> Unfortunately, I didn't save the log files so I can't quote the
>> error message from the template stuff...
>>
>>
>>> But if it's always 64-bit then you'd have to use Atomic::load to
>>> allow for 32-bit platforms.
>>
>> What's the official story on 32-bit platforms? Is that what
>> bit me in JPRT? i.e., did we still have a 32-bit ARM build
>> config'ed in JPRT a month or two ago???
>>
>>
>>> These counts etc are all just for statistics so we aren't really
>>> concerned about eventual overflows in long running VMs - right?
>>
>> Yup these are just statistics... overflow is not a killer.
>>
>>
>>>
>>> ---
>>>
>>> // # 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.
>>
>> Coleen asked for the new comments in thread.cpp to be moved
>> over to column 1. She asked for the new comments in thread.hpp
>> to be indented with more spaces. I started with 2 spaces and
>> the variables still didn't stand out. I tried 4 spaces and
>> they still didn't stand out probably because of the leading
>> _smr_... So I went with 8 spaces...
>
> Since you have the same but more indepth comments in the .cpp file,
> and at least for my sampling the comments in the .hpp file look
> redundant, I think you should not have the same comments in the .hpp
> file. They're really values that the implementation uses, not part
> of the interface. My vote is to remove the comments from the .hpp file.
The first line is intended to be a 1-line summary and it is identical
between thread.cpp and thread.hpp. My thinking was that someone would
look in thread.hpp first to see what the field was for and if they
wanted the gory details about the field they could look at the impl
notes in thread.cpp...
Of course, that creates a maintenance problem in keeping the 1-liners
in sync between thread.hpp and thread.cpp. I'm okay with deleting the
1-liners from thread.hpp if folks think they should go...
Dan
>
> Coleen
>>
>>
>>>
>>>> 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!
>>
>> Yes, a new pair of eyes on this code is always useful.
>>
>>
>>>
>>>> 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 :)
>>
>> Will fix. I tried typing both and neither looked right to me.
>> (I might be getting blurry eyed with this stuff)...
>> So I went with the spelling from Coleen's comment... :-)
>>
>> Thanks for taking another look!!
>>
>> Dan
>>
>>
>>>
>>> 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 hotspot-runtime-dev
mailing list