<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<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/~rkennke/8180932/webrev.09/">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/"><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? Because, from the top of my head, I can name 3
source files all in entirely different styles ;-)<br>
<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 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 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>
<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 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>
</body>
</html>