Request for review (s): 6814390 G1: remove the concept of non-generational G1

[Tony Printezis]

Bengt,

Many thanks for doing this cleanup! The changes look good, but it turns 
out you can remove a few more things:

g1CollectorPolicy.hpp:

   double _mark_init_start_sec;
   TruncatedSeq* _concurrent_mark_init_times_ms;
   double predict_init_time_ms() {
     return get_new_prediction(_concurrent_mark_init_times_ms);
   }

and their uses.

Can you also maybe rename the following (the "pre" suffix does not make 
sense any more)?

G1CollectorPolicy::record_concurrent_mark_init_end_pre()

to

G1CollectorPolicy::record_concurrent_mark_init_end()

Thanks,

Tony

On 08/18/2011 04:44 AM, Bengt Rutisson wrote:
>
> Hi Ramki,
>
> On 2011-08-18 10:10, Y. Srinivas Ramakrishna wrote:
>> Hi Bengt -- that should also make the method checkpointRootsInitial() 
>> dead, and
>> thus also record_concurrent_mark_init_start() [The clue is the guarantee
>> you deleted in the latter method at line 940 which would be violated 
>> in the case of G1Gen
>> because it would be tautologically false now.] I think the dead code 
>> contagion
>> does stop there, as far as i could tell from a browse of the code. 
>> Perhaps a good
>> IDE may find you a few more dead methods, who knows...
>
> Wow! This change really propagates. Thanks for catching this. I found 
> a couple of more methods to delete and one class. Now I hope I got all 
> of the dead code that this change generated.
>
> This is what I deleted this time around:
>
> G1CollectorPolicy::record_concurrent_mark_init_start()
> G1CollectorPolicy::record_concurrent_mark_init_end()
> ConcurrentMark::checkpointRootsInitial()
> G1CollectedHeap::do_sync_mark()
> class CMMarkRootsClosure
>
> Here is an updated webrev:
> http://cr.openjdk.java.net/~brutisso/6814390c/webrev.03/
>
> Thanks,
> Bengt
>
>>
>> Rest looks good to me.
>> -- ramki
>>
>> On 8/18/2011 12:28 AM, Bengt Rutisson wrote:
>>>
>>> Hi John,
>>>
>>> Thanks for the review!
>>>
>>>> Looks good to me. You may want to consider removing the 
>>>> CMCheckpointRootsInitialClosure - it's only instantiated in the 
>>>> code you removed from concurrentMarkThread.cpp.
>>>
>>> Good point. I removed the CMCheckpointRootsInitialClosure class. 
>>> Here is an updated webrev:
>>>
>>> http://cr.openjdk.java.net/~brutisso/6814390c/webrev.02/
>>>
>>> Bengt
>>>
>>>>
>>>> JohnC
>>>>
>>>> On 08/16/11 11:03, Bengt Rutisson wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Could I have a couple of reviews for this change?
>>>>>
>>>>> Background:
>>>>> G1 was originally designed to be able to run in a non-generational 
>>>>> mode. This has not been used for a long time and no testing has 
>>>>> been done. Thus, the code has bit rotted. We don't see the need 
>>>>> for this feature anymore, so rather than fixing it we should 
>>>>> remove it.
>>>>>
>>>>> This is actually a low priority item, but since it will make my 
>>>>> next step (supporting young space sizing better) simpler I would 
>>>>> like to get this CR out of the way.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~brutisso/6814390c/webrev.01/
>>>>>
>>>>> CR:
>>>>> 6814390 G1: remove the concept of non-generational G1
>>>>> http://monaco.us.oracle.com/detail.jsf?cr=6814390
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>>
>>>>
>>>
>>
>