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

Derek White derek.white at oracle.com
Thu Mar 17 22:45:09 UTC 2016

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::collectionsetiteratefrom() 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::doheapregion() 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::doheapregion(AbortableHeapRegionClosure, *): bool doheapregion(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>

More information about the hotspot-gc-dev mailing list