RFR: 8075215: SATB buffer processing found reclaimed humongous object

[Bengt Rutisson]

Hi Kim,

On 30/04/15 19:17, Kim Barrett wrote:
> On Apr 30, 2015, at 8:17 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>> Over all the fix looks good. Nice work!
> Thanks for looking at this.
>
>> A couple of minor style questions. Feel free to ignore them:
>>
>> concurrentMark.cpp:
>>
>> 2588   CMSATBBufferClosure _cm_satb;
>>
>> Do you mind calling the variable _cm_satb_cl instead? I think that is more consistent with the other two closures declared in the same place, _cm_cl and _code_cl.
> Good idea.
>
>> 2594   G1RemarkThreadsClosure(G1CollectedHeap* g1h, CMTask* task) :
>> 2595     _cm_satb(task, g1h), _cm_cl(g1h, g1h->concurrent_mark(), task),
>> 2596     _code_cl(&_cm_cl, !CodeBlobToOopClosure::FixRelocations),
>>
>> To me this looks like two instance variables are being initialized when in fact there are three. I would prefer a line break between the first and the second, i.e:
> Good idea.  I didn’t like that either, but had left it alone.
>
>> concurrentMark.inline.hpp
>>
>> 292 inline void CMTask::make_reference_grey(oop obj, HeapRegion* hr) {
>> 293   if (_cm->par_mark_and_count(obj, hr, _marked_bytes_array, _card_bm)) {
>>             ...
>> 341   }
>> 342 }
>>
>> I think I would prefer an early exit rather than having the whole method nested one indentation level.
> I tried that and personally didn’t like it as much.  There’s another place later where a similar change should be made for consistency, for the call to is_below_finger.  I generally dislike introducing one immediate use variables like below, but didn’t care for the two negated conditional expressions to short circuit out either, while the additional indentation levels don’t bother me here.  Maybe my Lisp accent is showing.

Ok. That's fine.

>
>> So, something like:
>>
>> inline void CMTask::make_reference_grey(oop obj, HeapRegion* hr) {
>>   bool marked = _cm->par_mark_and_count(obj, hr, _marked_bytes_array, _card_bm);
>>   if (!marked) {
>>      // Another thread beat us to this object.
>>      return;
>> }
>>   ...
>> }
>>
>>
>> satbQueue.cpp
>>
>> // If G1SATBBufferEnqueueingThresholdPercent == 0 we could skip filtering.
>>
>> Do you want to fix this too in a separate change? In that case, will you file a bug to track that?
> https://bugs.openjdk.java.net/browse/JDK-8079167
>
> In the process of filing that bug I noticed the documentation for that configuration option says the filtering should not occur when the value is 0.  The old comment was correct that we couldn’t avoid the filtering with the old completed buffer handling.  We can now, but I don’t want to make that change as part of this change set.  I’d like to test and look at the performance impact before making it.

Thanks for filing the bug. I agree that it is good to do some 
performance evaluation before pushing that change. I also don't think is 
a very urgent bug. My guess is that we normally benefit from doing 
filtering, that is to have G1SATBBufferEnqueueingThresholdPercent > 0, 
but it would make sense to fix it anyway just to be more clear about how 
the filtering works.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8075215
>
> Updated webrev:
> http://cr.openjdk.java.net/~kbarrett/8075215/webrev.01/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~kbarrett/8075215/webrev.01.incr/
>

Looks good. Reviewed.

Bengt

>