CRR (M): 7127697: G1: remove dead code after recent concurrent mark changes

[Tony Printezis]

Thanks Bengt.

On 03/28/2012 07:40 AM, Bengt Rutisson wrote:
>
> Tony,
>
> Looks good. Thumbs up!
>
> Bengt
>
> On 2012-03-27 18:34, Tony Printezis wrote:
>> Bengt,
>>
>> Thanks for looking at it. Inline.
>>
>> On 03/27/2012 08:19 AM, Bengt Rutisson wrote:
>>>
>>> Hi Tony,
>>>
>>> It is always difficult to review removed code, but this looks good 
>>> to me.
>>
>> Thanks.
>>
>>> I would like to include your second change as well. It would be good 
>>> to remove BitMap::mostly_disjoint_range_union(). I realize it is a 
>>> difficult method to re-implement, but leaving it in without any 
>>> usage will make it break sooner or later anyway. So, better to 
>>> re-implement it if we ever need it again.
>>
>> Well, re-implementing it is a bit of a waste of time. Maybe, it could 
>> be revived from the hg history...
>>
>>> A couple of minor comments:
>>>
>>> concurrentMark.cpp
>>>
>>> Comment starting on line 2832 needs to be updated now that 
>>> drainAllSATBBuffers is gone. Maybe the whole comment is irrelevant?
>>>
>>> // This note is for drainAllSATBBuffers and the code in between.
>>> // In the future we could reuse a task to do this work during an
>>> // evacuation pause (since now tasks are not active and can be claimed
>>> // during an evacuation pause). This was a late change to the code and
>>> // is currently not being taken advantage of.
>>
>> Thanks for pointing this out. It turns out that the method is 
>> actually unused now, so it's also gone along with the comment. ;-)
>>
>>> heapRegion.hpp
>>>
>>> You removed CompleteMarkCSetClaimValue = 6 from enum ClaimValues. 
>>> Any reason why you did not renumber the entries following it?
>>
>> Laziness? :-)
>>
>>> In fact, why do the entries have numbers at all since they are 
>>> starting on 0 and just increasing by 1. That is the default for an 
>>> enum, right?
>>
>> Not sure why it was done this way. It's nice to have the numbers, 
>> say, during debugging (so you don't have to count to find out what a 
>> value of '3' means). I left them as is and I renumbered the last three.
>>
>> Thanks again for the code review! The new webrevs are here:
>>
>> http://cr.openjdk.java.net/~tonyp/7127697/webrev.1/webrev.0.G1CMCleanup/
>>
>> http://cr.openjdk.java.net/~tonyp/7127697/webrev.1/webrev.1.G1CMCleanupExtra/ 
>>
>>
>> Tony
>>
>>> Bengt
>>>
>>> On 2012-03-27 11:29, Tony Printezis wrote:
>>>> Hi all,
>>>>
>>>> I'd like a couple of code reviews for this set of changes:
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7127697/webrev.0/webrev.0.G1CMCleanup/ 
>>>>
>>>>
>>>> which removes lots code from conc mark that was made unreachable 
>>>> from a recent set of changes. The changes are mostly code removal 
>>>> with only a very small number of code actually modified.
>>>>
>>>> This is an additional proposed change:
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7127697/webrev.0/webrev.1.G1CMCleanupExtra/ 
>>>>
>>>>
>>>> to remove a method from the bitmap class which is not used any more 
>>>> (it was only used by the code in G1 conc mark that I'm removing). 
>>>> I'd like some feedback on whether I should go ahead and remove it 
>>>> or leave it in in case it's used in the future.
>>>>
>>>> Tony
>>>>
>>>
>