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.daughe...@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
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
