code question

[Joseph Provino]

I made changes for AbortableHeapClosure.

- classes derived from HeapClosure in which doHeapRegion always returns 
false
   are changed from bool doHeapRegion to void doHeapRegion.

- classes with a doHeapRegion which sometimes returns true now derive from
   AbortableHeapClosure and have bool doHeapRegionAbortable.

- At first I had AbortableHeapClosure be a subclass of HeapClosure.
   But I think it's better that AbortableHeapClosure is not a subclass 
of HeapClosure
   so methods which take a HeapClosure won't also accept an 
AbortableHeapClosure.
   I think this means duplication some methods such as

   void G1CollectedHeap::heap_region_iterate(HeapRegionClosure* cl) const {
     _hrm.iterate(cl);
   }
   
+ void G1CollectedHeap::heap_region_iterate(AbortableHeapRegionClosure* cl) const {
+   _hrm.iterate(cl);
+ }


void HeapRegionManager::iterate(RegionClosure* blk)
void HeapRegionManager::iterate(AbortableHeapRegionClosure* blk)

There's probably a better c++ way to this but I don't know it.

Here's a webrev if you want to take a quick look:

http://bussund0416.us.oracle.com/export/users/jprovino/hs-rt-closure/hotspot/webrev

joe






http://bussund0416.us.oracle.com/export/users/jprovino/hs-rt-closure/hotspot/webrev/
On 6/8/2015 10:44 AM, Stefan Karlsson wrote:
> On 2015-06-08 16:37, Thomas Schatzl wrote:
>> Hi Joe,
>>
>> On Thu, 2015-06-04 at 15:12 -0400, Joseph Provino wrote:
>>> I just ran across some code in
>>> g1CollectedHeap::collection_set_iterate_from()
>>> that looks a little strange.  It's been there since the first 
>>> revision 342.
>>>
>>>       if (cl->doHeapRegion(cur) && false) {
>>>         cl->incomplete();
>>>         return;
>>>       }
>>>
>>> Is there any reason why this isn't just
>>>
>>>         cl->doHeapRegion(cur);
>>    file a bug and enjoy an easy cleanup for your path to a Committer...
>>
>> The HeapRegionClosure has a way of sending an "abort" notification due
>> to error to the caller, which is that "cl->incomplete()" call.
>> However in many if not all cases it is not used because either the work
>> that needs to be done cannot fail, or any failure is really fatal
>> anyway.
>>
>> That's probably why the strange code - but you'll probably find more of
>> that the deeper you dig.
>>
>> I would replace that code by the call to cl->doHeapRegion() and
>> guarantee() that its return value is always true in this case.
>
> You should probably change G1CollectedHeap::collection_set_iteratate 
> as well, since none of the callers pass a closure that returns true 
> from doHeapRegion.
>
> Erik H suggested that we should create new closure named 
> AbortableHeapRegionClosure and get rid of the abort mechanism from 
> HeapRegionClosure.
>
> StefanK
>
>>
>> Thanks,
>>    Thomas
>>
>>
>

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