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

[Derek White]

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()?

    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/58b4dc65/attachment.htm>