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

[Derek White]

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
>
>   - Line 267: too many b's
>      - Does iterate really call doHeapRegionAbortabble()? (sp)
>
> heapRegionManager.inline.hpp
>  - Odd spacing at line 102.
>
>  - 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?
>
> OK, what were we simplifying again in this RFE? :-)
>
>  - Derek

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