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

[Bengt Rutisson]

Tony,

Thanks for the review!

Nice that you found even more code to delete! I did the changes you 
suggested. Here is an updated webrev:

http://cr.openjdk.java.net/~brutisso/6814390c/webrev.04/

Thanks,
Bengt


On 2011-08-18 19:08, Tony Printezis wrote:
> Bengt,
>
> Could I ask a quick favor for an additional (embarrassing) 
> refactoring? Given your changes I don't think the following is needed:
>
> --- a/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp
> +++ b/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp
> @@ -2875,13 +2875,6 @@
>    // We are doing young collections so reset this.
>    non_young_start_time_sec = young_end_time_sec;
>
> -  // Note we can use either _collection_set_size or
> -  // _young_cset_length here
> -  if (_collection_set_size > 0 && _last_young_gc_full) {
> -    // don't bother adding more regions...
> -    goto choose_collection_set_end;
> -  }
> -
>    if (!full_young_gcs()) {
>      bool should_continue = true;
>      NumberSeq seq;
> @@ -2915,7 +2908,6 @@
>        _should_revert_to_full_young_gcs  = true;
>    }
>
> -choose_collection_set_end:
>    stop_incremental_cset_building();
>
>
> Tony
>
> On 08/18/2011 12:54 PM, Tony Printezis wrote:
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>