RFR (XL): 8218668: Clean up evacuation of optional collection set
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Thu Apr 4 21:41:00 UTC 2019
Hi Thomas,
On 4/4/19 4:18 AM, Thomas Schatzl wrote:
> Hi Kim,
>
> On Tue, 2019-04-02 at 21:02 -0400, Kim Barrett wrote:
>>> On Mar 13, 2019, at 9:12 AM, Thomas Schatzl <
>>> thomas.schatzl at oracle.com> wrote:
>>>
>>> Hi all,
>>>
>>> I fixed some time measurement related issues in a new webrev:
>>>
>>> - do not use Tickspan.milliseconds() in measuring time because it
>>> truncates the result to integers :( Using "seconds() * 1000.0" as
>>> workaround for now, need to figure out what to do here.
>>>
>>> - during optional evacuation the change added initial evacuation
>>> time too
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.0_to_1/
>>> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.1/
>> I've been struggling a lot with nomenclature in this review, but I
>> don't have a well formed description of what I'm tripping over, let
>> alone any concrete suggestions for changes. I think it might help if
>> there was a term for the non-optional part of the collection set; a
>> point of confusion for me is whether something is about that or the
>> complete collection set.
> They are called "initial old regions" in the code, however they do not
> play a huge role.
>
> Let me digress about the naming in the code which I tried to adhere
> to:
>
> - the collection set consists of "young part" and "old part"
> - the "young part" consists of eden and survivor regions (which are
> relabeled as eden almost immediately).
> - the "old part" itself consists of "initial old regions" and "optional
> [old] regions".
> - the "initial collection set" consists of "young part" and "initial
> old regions", collected by the "initial evacuation" phase.
> - the "optional collection set" is a set of increments over the
> "optional old regions" that is collected in the "optional evacuation"
> phase.
>
> However I found it better to think of increments of the collection set,
> the parts the next evacuation will work on. Then there is only the
> "current increment" and "all" parts of the collection set to think
> about than to worry about what it is actually in a particular increment
> as the actual copying handles them completely the same.
>
> For the "initial evacuation" phase, the increment is survivor + eden +
> initial old regions, for the "optional evacuation" phase, which is a
> sequence of evacuations of parts over the "optional old regions", each
> part being the increment that is currently worked on.
>
> There were some outdated comments that might have confused you :( I
> updated the webrev with better comments and hopefully improved and more
> conistent naming.
>
> There is now also a huge comment in g1CollectionSet.hpp that gives an
> overview of how the collection set looks like and changes over time,
> with examples.
>
>> Despite that, I think generally a nice improvement.
>>
> Thanks.
>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1Policy.cpp
>> 662 cost_per_entry_ms =
>> (average_time_ms(G1GCPhaseTimes::ScanRS) +
>> average_time_ms(G1GCPhaseTimes::OptScanRS)) / (double) cards_scanned;
>>
>> We have this_pause_was_young_only. Perhaps use that to not compute
>> the average for OptScanRS?
> I changed the code to only calculate the average if the collection had
> been a mixed collection. Originally I simply did not see a performance
> problem with that.
>
>> -------------------------------------------------------------------
>> -----------
>> [pre-existing] adaptive_young_list_length() seems like a bad name for
>> a predicate. Maybe add "use_" prefix or something like that? This
>> could be a separate RFE.
> I will fix this separately.
>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1Policy.cpp
>> 1191 void
>> G1Policy::select_old_collection_set_regions(G1CollectionSetCandidates
>> * candidates,
>> 1192 double
>> time_remaining_ms,
>> 1193 uint&
>> num_expensive_regions,
>> 1194 uint&
>> num_initial_regions,
>> 1195 uint&
>> num_optional_regions) {
>>
>> num_expensive_regions is not used by the caller. It is only used
>> locally for logging.
>>
> Removed. Oversight from moving code around...
>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1Policy.cpp
>> 1191 void
>> G1Policy::select_old_collection_set_regions(G1CollectionSetCandidates
>> * candidates,
>> ...
>>
>> I'm wondering if things might be simpler if the API here was instead
>>
>> select_old_collection_set_regions(collection_set, candidates,
>> time_remaining)
>>
>> (where I'm not sure "select" is the right word)
> Renamed to "calculate"; so you suggest that the policy immediately puts
> the results of that method into the collection set?
>
> I thought it would be easier to understand if, like the current code
> does, just calculates which regions from the candidate set we are going
> to process in this collection (i.e. the "initial old regions" and the
> "optional old regions") given the time limit, and clearly separate that
> from doing the legwork to add these to the correct sets and do other
> preparation.
>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>> 745 // Evacuate the next set of optional regions.
>> 746 void evacuate_next_optional_regions(G1ParScanThreadStateSet*
>> per_thread_states);
>>
>> This appears to be a helper function and shouldn't be public.
> Fixed. I moved another one or two methods to private.
>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1CollectionSet.hpp
>> 171 void initialize_optional(uint max_length);
>>
>> Declared, but no longer defined or used.
> Removed.
>
>> -------------------------------------------------------------------
>> -----------
>>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.1_to_2/
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.2/
Nice clean up!
The updated comment at G1CollectionSet is much better to me.
I have minor nits:
--------------------------------------------------
share/gc/g1/g1CollectedHeap.cpp
157 Tickspan G1CollectedHeap::run_task(AbstractGangTask* task, uint
num_workers) {
- Couldn't always use workers()->run_task(task) instead of num_workers
treatment? The difference between WorkGang::run_task(AbstractGangTask*
task) and G1ColectedHeap::run_task() looks like WorkGang doesn't allow
active workers to be 0. I don't think we have such case.
- Just an idea and not expect to fix here. It would nice if this api
exists on WorkGang something like run_task() and run_task_ms().
--------------------------------------------------
share/gc/g1/g1CollectionSet.hpp
52 // This set is built incrementally at mutator time as regions are
retired, and,
- two commas?
174 size_t _inc_part_start;
- Not initialized at ctor.
--------------------------------------------------
share/gc/g1/g1RemSet.cpp
342 void G1ScanRSForRegionClosure::scan_opt_rem_set_roots(HeapRegion* r){
- Space between '){'
--------------------------------------------------
share/gc/g1/g1OopStarChunkedList.cpp
- copyright year update
--------------------------------------------------
share/gc/g1/g1OopStarChunkedList.hpp
- copyright year update
Thanks,
Sangheon
>
> Thanks,
> Thomas
>
>
More information about the hotspot-gc-dev
mailing list