Hi Robin W!
Welcome to the Thread-SMR party!!
On 11/20/17 4:48 AM, Robin Westberg wrote:
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)).
Code reviews are always welcome (OpenJDK role does not matter).
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);
Nice catch! Fixed with your proposed change.
The careful reader will notice that in revoke_bias() we are not
creating a ThreadsListHandle that is in scope for the entire
function and we are doing cached monitor info walks without an
obvious ThreadsListHandle in place.
revoke_bias() is called at a safepoint from most locations. The
one call that is not made at a safepoint is from
BiasedLocking::revoke_and_rebias() and it is made by a JavaThread
that is revoking a bias on itself which is also safe.
We should add an assertion to revoke_bias() that looks something
like this:
assert(requesting_thread == Thread::current() ||
SafepointSynchronize::is_at_safepoint(),
"must be operating on self or at a safepoint.");
but we'll do that in a separate follow up bug since that will
require biased locking focused testing.
src/hotspot/share/runtime/memprofiler.cpp
55-56 grabs the Threads_lock, shouldn’t be needed anymore.
Nice catch! Deleting lines 55-56.
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?
Ouch! Looks like JavaThread::is_exiting() is right, but
JavaThread::is_terminated() has been inefficient all this
time. Fixed.
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?
It definitely needs some updating... Here's a stab at it:
// The Threads class links together all active threads, and provides
// operations over all threads. It is protected by the Threads_lock,
// which is also used in other global contexts like safepointing.
// ThreadsListHandles are used to safely perform operations on one
// or more threads without the risk of the thread exiting during the
// operation.
//
// Note: The Threads_lock is currently more widely used than we
// would like. We are actively migrating Threads_lock uses to other
// mechanisms in order to reduce Threads_lock contention.
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?
The hash_table_size parameter is currently unused. We were using
a different hash table before that allowed the size to be set.
Unfortunately, that hash table didn't support being freed so we
switched to ResourceHashtable.
We have a project note to come back and update the underlying
hash table to work with dynamic table sizes, but Erik hasn't
had the cycles to do it yet.
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.”
I went with this:
if (ThreadLocalHandshakes) {
// The thread is about to be deleted so cancel any handshake.
thread->cancel_handshake();
}
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?
Fixed.
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.
Agreed. Restored to the original.
Thanks for the review!
Dan
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
