[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    CRR (M): 7127697: G1: remove dead code after recent concurrent mark changes
From:       john.cuthbertson () oracle ! com (John Cuthbertson)
Date:       2012-03-28 16:57:14
Message-ID: 4F7342EA.1080205 () oracle ! com
[Download RAW message or body]

Hi Tony,

I looked at the two new webrevs and they look good. This is a massive 
cleanup. The copyrights in bitMap.hpp and bitMap.cpp need updating.

JohnC

On 3/27/2012 9:34 AM, 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
>>>
>>


[prev in list] [next in list] [prev in thread] [next in thread]