<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi Kim and Jungwoo,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-07 02:36, Jungwoo Ha wrote:<br>
</div>
<blockquote
cite="mid:CA+n_jhh_GrnQ5MNWB1B13UaRGonQmdGXeDz=3W4WyzOGVP0J0w@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
</span>------------------------------------------------------------------------------<br>
<br>
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp<br>
271 // Support for CMSFastPromotionFailure<br>
272 reset_promotion_failed();<br>
<br>
Why this and not just initialize the data member in the
constructor<br>
initializer list?<br>
<br>
</blockquote>
<div><br>
</div>
<div>Okay. I will fix that.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
------------------------------------------------------------------------------<br>
<br>
[All of code discussed here is in par_promote().]<br>
<br>
The tests of CMSFastPromotionFailure &&
has_promotion_failed() that<br>
are outside the lock have a sort of DCLP (double checked
locking<br>
pattern) feel to them. The goal here is to avoid the
locked region<br>
and go do something else, instead of waiting for the
current lock<br>
holder to complete the work the checker would be doing if
only they<br>
could get the lock.<br>
<br>
I think I would like it better if the outside the lock
tests were<br>
moved closer to the lock entries, to emphasize that
relationship, and<br>
to reduce the code affected by the introduction of this
set of<br>
changes. That might result in the thread that gives up
doing a little<br>
more work before giving up. I'm particularly thinking of<br>
<br>
1358 if (CMSFastPromotionFailure &&
has_promotion_failed()) {<br>
1359 return NULL;<br>
1360 }<br>
<br>
removal of which would let execution proceed to<br>
<br>
1373 if (!expand_and_ensure_spooling_space(promoInfo))
{<br>
1374 return NULL;<br>
<br>
and give up because the moved test caused expand_xxx to
return false.<br>
I don't know if the placement of the test at line 1358
rather than<br>
inside expand_and_ensure_spooling_space() would be
significant. I<br>
suspect not, but without measurements I don't have a lot
of confidence<br>
in that supposition. But I think that change would
improve the<br>
understandability of the code.<br>
<br>
In the other case,<br>
<br>
1381 if (CMSFastPromotionFailure &&
has_promotion_failed()) {<br>
1382 return NULL;<br>
1383 }<br>
1384 obj_ptr = expand_and_par_lab_allocate(ps,
alloc_sz);<br>
<br>
I would definitely prefer that test be moved into<br>
expand_and_par_lab_allocate(). This is the only call to
that<br>
function, and the first thing it does is lock the mutex
and re-perform<br>
the test.<br>
</blockquote>
<div><br>
</div>
<div>These are really important for performance. If we
remove line 1358, the pause time with the given example
goes up by 2x.</div>
<div>I'll move 1381 to inside expand_and_par_lab_allocate().</div>
<div> <br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
------------------------------------------------------------------------------<br>
<br>
I'm suspicious of the during-review change to make<br>
_has_promotion_failed non-atomic.<br>
<br>
I see the argument for the interaction between set and
test; the set<br>
occurs in lock context so gets released on the way out of
the lock,<br>
while the first time each core gets a stale false value it
will<br>
acquire the lock and acquire the updated set value, so
that future<br>
tests on the same core will avoid the code in question,
until the<br>
reset finally happens.<br>
<br>
However, I think the present interaction between reset and
test is not<br>
safe. A test seeing a stale true value leads to an
inappropriate skip<br>
of the protected code, with nothing obvious to prevent
that from<br>
occurring "forever".<br>
<br>
------------------------------------------------------------------------------<br>
</blockquote>
<div><br>
</div>
<div>I am sure the code is correct under x86 as is, but
please let me know how to fix it for other architecture. </div>
<div><br>
</div>
<div>I'll post the update after the discussion is settled.</div>
</div>
</div>
</div>
</blockquote>
<br>
I don't think a thread can ever see a stale true value. We only
reset after a GC and there is a lot of things happening, including
several locks being taken and released, before we start a new GC.
So, as far as I can tell this does not require any more barriers. I
may be missing something though. If I am, can you give a more
specific example, Kim?<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote
cite="mid:CA+n_jhh_GrnQ5MNWB1B13UaRGonQmdGXeDz=3W4WyzOGVP0J0w@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>--Jungwoo</div>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>