RFR (M): 8027962: Per-phase timing measurements for strong roots processing

[Kim Barrett]

On Feb 18, 2015, at 9:36 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Bengt,
> 
>  thanks for the review.
> 
> On Tue, 2015-02-17 at 16:03 +0100, Bengt Rutisson wrote:
>>> JIRA:
>>> https://bugs.openjdk.java.net/browse/JDK-8027962
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8027962/webrev/
>> 
>> I think it would be good if we can make sure that the GC code does not 
>> leak in to runtime/timer. Not sure exactly how, but it would be good to 
>> try some change out to avoid this dependency.
> 
> Actually there is a really simple way of doing that by declaring an
> interface with the required methods and use that.
> 
>> TestGCLogMessages.java
>> 
>> I find it a little confusing that the instance variable messages has the 
>> same name as the parameter to the method checkMessagesAtLevel(). Could 
>> you change the name of one of them?
>> 
>>   47     LogMessageWithLevel messages[] = new LogMessageWithLevel[] {
>> 
>>   79     void checkMessagesAtLevel(OutputAnalyzer output, 
>> LogMessageWithLevel messages[], int level) throws Exception {
>> 
>> Also, what do you think about using an enum instead of using the integer 
>> values 2, 3 etc for levels?
> 
> All fixed.
> 
> New webrevs at:
> http://cr.openjdk.java.net/~tschatzl/8027962/webrev.1/ (full)
> http://cr.openjdk.java.net/~tschatzl/8027962/webrev.0_to_1/
> (incremental)

I think it's not very nice that TrackPhaseTime needs to deal with a
possibly null _data and that SharedHeap::process_xxx_roots have a
default NULL for the new PhaseTimeData argument. Obviously this is
because only G1 support for reporting this additional phase
information is being implemented by this change set. And the approach
taken is fine, given that limitation. Is the intention to provide it
for other collectors later? (And is there an RFE?) 8027962 talks
about improving for "at least the G1 collector", but it seems like
other collectors benefit similarly.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp 
 354 static const char* g1_ext_root_task_strings[G1CollectedHeap::G1H_PS_NumElements] = {
 355   "Filter SATB Roots (ms)",
 356   "CM RefProcessor Roots (ms)",
 357   "Wait For Strong CLD (ms)",
 358   "Weak CLD Roots (ms)"
 359 };

I would drop the explicit array size from the declaration, and instead
add some static asserts to verify all present and properly accounted
for, e.g.

STATIC_ASSERT(G1CollectedHeap::G1H_PS_NumElements == ARRAY_SIZE(g1_ext_root_task_strings));
STATIC_ASSERT(G1CollectedHeap::G1H_PS_filter_satb_buffers == 0);
...

Similarly for the SharedHeap task enum.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp 
 994     G1H_PS_First,

Is there a reason why this doesn't start at
SharedHeap::SH_PS_NumElements?  That seems more natural to me.

[As an aside, C++11 has a nice facility for mapping from enum "codes"
to strings, including hierarchies of code sets: the std::error_code
and std::error_category classes.  (Arguably poorly named; they can be
used for things other than errors.)  Too bad we have to reinvent
wheels.]

------------------------------------------------------------------------------