RFR (S) JDK-8085983, G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified

[Joseph Provino]

On 3/17/2016 6:45 PM, Derek White wrote:
> Possible pre-existing bug, see below...
>
> On 3/17/16 6:34 PM, Derek White wrote:
>> On 3/10/16 3:10 PM, Joseph Provino wrote:
>>> Please review these changes.
>>>
>>> CR: JDK-8085983 G1CollectedHeap::collection_set_iterate_from() has 
>>> unused code and can be simplified 
>>> <https://bugs.openjdk.java.net/browse/JDK-8085983>
>>>
>>> Webrev: http://cr.openjdk.java.net/~jprovino/8085983/webrev.02
>>>
>>> Thanks.
>>>
>>> joe
>> Hi Joe,
>> Here are my comments.  The last issue looks serious, but I suggested 
>> several possible solutions.
>>
>> heapRegionManager.hpp
>>  - Copyright
Fixed.
>>
>>   - Line 267: too many b's
>>      - Does iterate really call doHeapRegionAbortabble()? (sp)
Fixed.  Good eyes.  ;-)
>>
>> heapRegionManager.inline.hpp
>>  - Odd spacing at line 102.
Fixed.
>>
>>  - In HeapRegionManager::iterate(), line 118:
>>    - Should be calling HeapRegionManager::do_heap_region() here, so 
>> iterate can abort?
>>       Wait! What happened to the call to incomplete()?
>
> And why didn't the old (or new) par_iterate() call blk->incomplete(); 
> when bailing out of iteration? I think the current callers of 
> par_iterate() don't check for complete() iterations, but that's 
> waiting to happen.
>> Wait! not all Closure objects have an incomplete() method, only
>>    AbortableHeapRegionClosures.
>>
>>    OK, one fix would be to catch the abort condition in
>>    HeapRegionManager::do_heap_region(AbortableHeapRegionClosure, *):
>>
>>     bool do_heap_region(AbortableHeapRegionClosure* blk, HeapRegion* 
>> r) const {
>>       bool res = blk->doHeapRegion(r);
>>       if (res) {
>>         blk->incomplete();
>>       }
>>       return res;
>>     }
>>
>>    But this means that the _complete field is only valid if
>>    AbortableHeapRegionClosure::doHeapRegion() is only called from the
>>    new method above. Could enforce this by making doHeapRegion()
>>    private and making HeapRegionManager a friend.
>>
>>    Alternatively, if we want doHeapRegion() callable from other
>>    places, we could have a public non-virtual doHeapRegion() method
>>    that is a wrapper around a protected virtual doHeapRegionWork()
>>    method. The wrapper calls  doHeapRegionWork() and calls
>>    incomplete() if it return TRUE. But this means renaming all of the
>>    overriden method implementations (or could do it in reverse and
>>    rename the wrapper and all of it's callers).
>>
>>    Alternatively (3rd option), we could make the
>>    AbortableHeapRegionClosure::doHeapRegion() overrides all 
>> responsible for
>>    calling incomplete() if they return true.
>>
>>    Anyone have an opinion here? Am I calling wolf on this issue?
I'll look at this in more detail.  Seems there's a problem and it's 
going to get more complicated.
>>
>> OK, what were we simplifying again in this RFE? :-)
Good question!

Thanks.

joe
>>
>>  - Derek
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160318/995ab30a/attachment.htm>