<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><br></div><div>--Jungwoo</div></div><br></div></div>