<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

I also agree that adding the assert(!<br>
SafepointSynchronize::is_at_<u></u>safepoint()) (or assert(!<br>
Thread->current()->is_VM_<u></u>thread())) may be good enough and if sneaking<br>
can be turned off for a case like this, it'd be safer.<br>
</blockquote>
I think we should go for this solution, i.e. adding the assert (because<br>
it would be wrong anyway to call this code during safepoint) and using<br>
try_lock()/unlock().<br>
<br>
Try_lock() itself is the same as the current code, and unlock() too<br>
(except for some additional checks that should fail fast). The code that<br>
is guarded handles occasional skipping already and actually exits fast<br>
if the frequency is too high, so I do not see a big advantage doing<br>
custom code.</blockquote></div></div></blockquote><div><br></div><div style>I tried a version with try_lock()/unlock() as in:</div><div style><br></div><div style><div><div>void CMSCollector::sample_eden_chunk() {</div><div>
  assert(!SafepointSynchronize::is_at_safepoint(), "Should not be at a safepoint.");</div><div>  if (CMSEdenChunksRecordAlways && _eden_chunk_array != NULL) {</div><div>    if (_eden_chunk_lock->try_lock()) {</div>
<div>      // Record a sample. This is the critical section. The contents</div><div>      // of the _eden_chunk_array have to be non-decreasing in the</div><div>      // address order.</div><div>      _eden_chunk_array[_eden_chunk_index] = *_top_addr;</div>
<div>      assert(_eden_chunk_array[_eden_chunk_index] <= *_end_addr,</div><div>             "Unexpected state of Eden");</div><div>      if (_eden_chunk_index == 0 ||</div><div>          ((_eden_chunk_array[_eden_chunk_index] > _eden_chunk_array[_eden_chunk_index-1]) &&</div>
<div>           (pointer_delta(_eden_chunk_array[_eden_chunk_index],</div><div>                          _eden_chunk_array[_eden_chunk_index-1]) >= CMSSamplingGrain))) {</div><div>        _eden_chunk_index++;  // commit sample</div>
<div>      }</div><div>      _eden_chunk_lock->unlock();</div><div>    }</div><div>  }</div><div>}</div></div><div><br></div></div><div style>It appears that the VM thread can call this at a safepoint (a young gen collection) unfortunately, and the !<span style="color:rgb(80,0,80)">is_at_</span><u style="color:rgb(80,0,80)"></u><span style="color:rgb(80,0,80)">sa</span><span style="color:rgb(80,0,80)">fepoint() assert fails. Here's a </span><span style="color:rgb(80,0,80)">snippet of </span><span style="color:rgb(80,0,80)">the stack trace:</span></div>
<div style><br></div><div><div>#7  0xf64e0be4 in CMSCollector::sample_eden_chunk (this=0x709366b0)</div><div>#8  0xf652ef14 in DefNewGeneration::allocate (this=0xf5c42900, word_size=18, is_tlab=false)</div><div>#9  0xf6654c62 in GenCollectedHeap::attempt_allocation (this=0xf5c2c8a8, size=18, is_tlab=false, first_only=false)</div>
<div>#10 0xf646ec02 in GenCollectorPolicy::satisfy_failed_allocation (this=0xf5c2c7a8, size=18, is_tlab=false)</div><div>#11 0xf6cf4880 in VM_GenCollectForAllocation::doit (this=0x6b0fe718)</div><div>#12 0xf6d1d324 in VM_Operation::evaluate (this=0x6b0fe718)</div>
<div>#13 0xf6d1aa3c in VMThread::evaluate_operation (this=0x6e02c800, op=0x6b0fe718)</div><div>#14 0xf6d1b349 in VMThread::loop (this=0x6e02c800)</div><div>#15 0xf6d1b560 in VMThread::run (this=0x6e02c800)</div></div><div>
<br></div><div style>This seems to me like a valid allocation code path.</div><div style><br></div><div style>Now, if I comment out the assert, it seems to work (though I haven't tested it very long.) This may be good if in fact sneak won't happen with try_lock()/unlock() only.</div>
<div style><br></div><div style>I haven't tried this, but another potential approach might be to give up sampling (just return) if it's called by the VM thread at a safepoint, though the VM thread might allocate a large object, and the evenness of the sample distribution could suffer to some extent.</div>
<div style><br></div></div></div></div>