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:34:07 UTC 2016
- Previous message (by thread): RFR (S) JDK-8085983,G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified
- Next message (by thread): RFR (S) JDK-8085983, G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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::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>
- Previous message (by thread): RFR (S) JDK-8085983,G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified
- Next message (by thread): RFR (S) JDK-8085983, G1CollectedHeap::collection_set_iterate_from() has unused code and can be simplified
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]