<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 7/5/17 3:17 PM, Roman Kennke wrote:<br>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Am 05.07.2017 um 20:30 schrieb Daniel
D. Daugherty:<br>
</div>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">On
6/27/17 1:47 PM, Roman Kennke wrote: <br>
<blockquote type="cite">Hi Robbin, <br>
<br>
Ugh. Thanks for catching this. <br>
Problem was that I was accounting the thread-local deflations
twice: <br>
once in thread-local processing (basically a leftover from my
earlier <br>
attempt to implement this accounting) and then again in <br>
finish_deflate_idle_monitors(). Should be fixed here: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.09/"
moz-do-not-send="true">http://cr.openjdk.java.net/~rkennke/8180932/webrev.09/</a>
<br>
<a class="moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.09/"
moz-do-not-send="true"><http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.09/></a>
<br>
</blockquote>
<br>
Are you thinking that this fix resolves all three bugs: <br>
<br>
8132849 Increased stop time in cleanup phase because of
single-threaded <br>
walk of thread stacks in
NMethodSweeper::mark_active_nmethods() <br>
</blockquote>
Yes. It requires additional support code by a GC though to become
actually multithreaded.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
8153224 Monitor deflation prolong safepoints <br>
</blockquote>
Yes. But there's more that we want to do:<br>
- deflate monitors during GC thread scanning (this is a huge
winner)<br>
- ultimately, deflate monitors concurrently (a JEP is on the way
to address this)<br>
<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
8180932 Parallelize safepoint cleanup <br>
</blockquote>
Yes :-)<br>
<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">JDK-8132849
is assigned to Tobias; it would be good to get Tobias' <br>
review of this fix also. <br>
</blockquote>
Ok, I will reach out to him.<br>
<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">General
comments: <br>
- Please don't forget to update Copyright years as needed
before pushing <br>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com"><br>
src/share/vm/runtime/safepoint.hpp <br>
L78: enum SafepointCleanupTasks { <br>
You might want to add a comment here: <br>
// The enums are listed in the order of the tasks
when done serially. <br>
</blockquote>
Good idea. Done.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">src/share/vm/runtime/safepoint.cpp
<br>
L556: ! thread->is_Code_cache_sweeper_thread()) {
<br>
L581: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_DEFLATE_MONITORS))
{ <br>
L589: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_UPDATE_INLINE_CACHES))
{ <br>
L597: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_COMPILATION_POLICY))
{ <br>
L605: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_SYMBOL_TABLE_REHASH))
{ <br>
L615: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_STRING_TABLE_REHASH))
{ <br>
L625: if (!
_subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_CLD_PURGE))
{ <br>
nit: HotSpot style doesn't usually have a space after
unary '!'. <br>
</blockquote>
Ok, thanks! I didn't know that. Is there a document that describes
the Hotspot style?</blockquote>
<tt> <br>
There is such a document:<br>
<br>
<a class="moz-txt-link-freetext" href="https://wiki.openjdk.java.net/display/HotSpot/StyleGuide">https://wiki.openjdk.java.net/display/HotSpot/StyleGuide</a><br>
<br>
I believe John Rose is the usual maintainer of the doc...<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">Because,
from the top of my head, I can name 3 source files all in entirely
different styles ;-)<br>
</blockquote>
<tt> <br>
True, very true... unfortunately. I don't know if John's doc
mentions<br>
it, but a general rule is to follow the prevailing style in the
file.<br>
Sometime this is impossible because sometimes we see multiple
styles<br>
in the same file (and we pull our hair out)...<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com"> <br>
L638: // Various cleaning tasks that should be done
periodically at safepoints <br>
L641: // Prepare for monitor deflation <br>
nit: Please add a period to the end of these sentences.
<br>
<br>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">src/share/vm/runtime/sweeper.cpp
<br>
L205: // TODO: Is this really needed? <br>
L206: OrderAccess::storestore(); <br>
That's a good question. Looks like that storestore() was
<br>
added by this changeset: <br>
<br>
$ hg log -r 5357 src/share/vm/runtime/sweeper.cpp <br>
changeset: 5357:510fbd28919c <br>
user: anoll <br>
date: Fri Sep 27 10:50:55 2013 +0200 <br>
summary: 8020151: PSR:PERF Large performance
regressions when code cache is filled <br>
<br>
The changeset is not small and it looks like two <br>
OrderAccess::storestore() calls were added (and one <br>
load_ptr_acquire() was deleted): <br>
<br>
$ hg diff -r 5356 -r 5357 | grep OrderAccess <br>
+ OrderAccess::storestore(); <br>
- nmethod *code = (nmethod
*)OrderAccess::load_ptr_acquire(&_code); <br>
+ OrderAccess::storestore(); <br>
<br>
It could be that the storestore() is matching an
existing <br>
OrderAccess operation or it could have been added in an
<br>
abundance of caution. We definitely need a Compiler team
<br>
person to take a look here. <br>
</blockquote>
I looked around a little bit. As far as I can tell, all compiler
threads are stopped at a safepoint there. And I don't see anything
else that uses the affected fields during the safepoint. There's a
fence() before resuming safepointed threads. I think it should be
safe without storestore(), but would like to get confirmation from
compiler team too.<br>
</blockquote>
<tt> <br>
Good idea! :-)<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">src/share/vm/runtime/synchronizer.hpp
<br>
L36: int nInuse; // currently associated with
objects <br>
L37: int nInCirculation; // extant <br>
L38: int nScavenged; // reclaimed <br>
nit: Please add one more space before '//' on L36,L37. <br>
</blockquote>
Oops. Done.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">src/share/vm/runtime/synchronizer.cpp
<br>
L1663: // Walk a given monitor list, and deflate idle
monitors <br>
L1664: // The given list could be a per-thread list or a
global list <br>
L1665: // Caller acquires gListLock <br>
L1666: int
ObjectSynchronizer::deflate_monitor_list(ObjectMonitor**
listHeadp, <br>
<snip> <br>
L1802: int deflated_count =
deflate_monitor_list(thread->omInUseList_addr(),
&freeHeadp, &freeTailp); <br>
L1804: Thread::muxAcquire(&gListLock, "scavenge -
return"); <br>
The above deflate_monitor_list() now occurs outside of
the <br>
gListLock where the old code held the gListLock for this
call. <br>
<br>
Yes, it is operating on the thread local list, but what
keeps <br>
two different worker threads from trying to
deflate_monitor_list() <br>
on the same JavaThread at the same time? <br>
</blockquote>
The mechanics in Threads::parallel_java_threads_do() (which I
adapted from Threads::possibly_parallel_oops_do()) ensure that
each worker thread claims a Java thread before processing it. This
ensures that each Java thread is processed by exactly one worker
thread.<br>
</blockquote>
<tt> <br>
Cool. No race there.<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
Without the gListLock, I don't see how the worker threads <br>
avoid conflicting over the same thread local list.
Minimally, <br>
the comment on L1665 needs updating. <br>
</blockquote>
Okidoki, I added those blocks there:<br>
<br>
// In the case of parallel processing of thread local monitor
lists,<br>
// work is done by Threads::parallel_threads_do() which ensures
that<br>
// each Java thread is processed by exactly one worker thread, and<br>
// thus avoid conflicts that would arise when worker threads would<br>
// process the same monitor lists concurrently.<br>
//<br>
// See also ParallelSPCleanupTask and<br>
// SafepointSynchronizer::do_cleanup_tasks() in safepoint.cpp and<br>
// Threads::parallel_java_threads_do() in thread.cpp.<br>
</blockquote>
<tt> <br>
I like the comment. (Others may find it wordy, but my comments are<br>
often thought to be wordy...)<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com"> <br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com"> <br>
L1697: counters->nInuse = 0; // currently
associated with objects <br>
L1698: counters->nInCirculation = 0; // extant <br>
L1699: counters->nScavenged = 0; // reclaimed <br>
nit: Please add one more space before '//' on L1697,
L1698. <br>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
old L1698: int nInuse = 0; <br>
old L1713: int inUse = 0; <br>
Nice catch here. I've read this code countless times and
missed <br>
this bug until now. It explains why some of my Java
monitor testing <br>
had odd "in use" counts. <br>
</blockquote>
Hmm. I am not aware of a bug there. the inUse declaration was
unused, that is all (I think..)<br>
</blockquote>
<tt> <br>
You would have that that when I pasted the two lines into the
comment,<br>
I would have noticed the difference in the names... sigh...<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:8def1665-1fb3-c7a2-bc0d-0b63601a0c56@redhat.com">
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
L1797: if (! MonitorInUseLists) return; <br>
nit: HotSpot style doesn't usually have a space after
unary '!'. <br>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">
L1808: thread->omInUseCount-= deflated_count; <br>
nit: Please add a space before '-='. <br>
</blockquote>
Done. Also some lines up:<span id="l1681"><br>
<br>
gOmInUseCount-= deflated_count;</span><br>
<span id="l1681"></span><br>
<blockquote type="cite"
cite="mid:e27c9cc2-5209-e2ab-57a1-a21d0de8dd12@oracle.com">The
only comment I need resolved is about the locking for the thread
<br>
local deflate_monitor_list() call. Everything else is minor. <br>
</blockquote>
<br>
Thanks so much for the thorough review!<br>
<br>
So here's revision #11:<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.10/">http://cr.openjdk.java.net/~rkennke/8180932/webrev.10/</a><br>
<br>
Roman<br>
</blockquote>
<tt><br>
src/share/vm/runtime/synchronizer.cpp<br>
L1664: // Caller acquires gListLock.<br>
The new stuff you added below the existing comment is
fine.<br>
However, that existing comment is still wrong because the
caller<br>
doesn't always acquire gListLock. Perhaps:<br>
<br>
// Caller acquires gListLock when operating on a global
list.<br>
<br>
Thanks for making the changes.<br>
<br>
Thumbs up!<br>
<br>
Dan<br>
<br>
</tt>
</body>
</html>