RFR: JDK-8136991: [REDO] Additional number of processed references printed with -XX:+PrintReferenceGC after JDK-8047125

[Kim Barrett]

On Sep 25, 2015, at 4:27 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> On 2015-09-25 00:40, Kim Barrett wrote:
>> On Sep 23, 2015, at 8:19 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~brutisso/8136991/webrev.04/
>> ------------------------------------------------------------------------------
>> src/share/vm/gc/shared/referenceProcessor.cpp
>>  195   GCRefTraceTime(ReferenceType ref_type, bool doit, GCTimer* timer, GCId gc_id, size_t count) :
>>  196     _gc_trace_time(reference_type_to_short_string(ref_type), doit, false, timer, gc_id) {
>>  197     log_ref_count(count);
>>  198   }
>> 
>> The _gc_trace_time printing is controlled by the doit parameter, while
>> log_ref_count is (internally) controlled by PrintReferenceGC and
>> PrintGCDetails.  Those two are presently equivalent, but that isn't at
>> all obvious and could change later.  I think the call to log_ref_count
>> ought to be conditional on doit here.
>> 
>> If the call to log_ref_count for JNI Weak References were similarly
>> conditional on trace_time, then log_ref_count wouldn't need any
>> internal conditionalization at all.
> 
> I see what you mean. The problem is that the JNI Weak Reference logging is only done for non-product builds. That means that if I want to guard the call to log_ref there I no longer have a one liner and need to go to #ifndef PRODUCT instead of just using the NOT_PRODUCT macro. When I do the unified logging changes the log_ref function will go away and even the non-product guard can go away. So, I think this can be cleaned up by that change.
> 
> In the meantime I suggest that log_ref takes a parameter "doit" to make it clearer that it is the same as for the GCTraceTime. Are you ok with that?

Not that I want to encourage it, but it wouldn’t have been the first occurrence of complex code in a NOT_PRODUCT form rather than enclosed by #ifndef PRODUCT.

But I like what you did better than my suggestion.

>> ------------------------------------------------------------------------------
>> src/share/vm/gc/shared/referenceProcessor.cpp
>>  236     GCRefTraceTime tt(REF_SOFT, trace_time, gc_timer, gc_id, stats.soft_count());
>> 
>> I don't understand the point of the change here and in later similar
>> places to replace the string name of the reference type with the
>> REF_XXX reference type.  It is this change that seems to drive the
>> addition of reference_type_to_[short_]string.  And
>> reference_type_to_string appears to be unused in these changes.
> 
> You are right. I had some intermediate change where I needed to know the reference type, but as it is now this change is unnecessary. I reverted it.

Thanks.

>> ------------------------------------------------------------------------------
>> src/share/vm/gc/shared/referenceProcessor.cpp
>> [removed comment]
>>  246     // Process cleaners, but include them in phantom statistics.  We expect
>>  247     // Cleaner references to be temporary, and don't want to deal with
>>  248     // possible incompatibilities arising from making it more visible.
>> 
>> This comment was removed.  (Actually, it was moved to the earlier
>> capture of the counts.)  However, it is still applicable here in that
>> we include cleaner processing in the time associated with phantom
>> reference processing.
> 
> I added the comment back. But I also left the comment in its new place where we set up the statistics. Since I've already missed reading this comment once I think it could be good to have it in both places where we treat Cleaners specially.

Thanks.  That’s exactly what I was hoping for, but didn’t express clearly.

>> ------------------------------------------------------------------------------
>> Regarding GCRefTraceTime, GCCMTraceTime, GCTraceTime
>> 
>> Version 04 webrev has GCTraceTime derived from StackObj, with
>> GCRefTraceTime and GCCMTraceTime having a GCTraceTime member and
>> derived from nothing.
>> 
>> Being derived from nothing is supposed to be disallowed; quoting from
>> allocation.hpp:
>> 
>>   All classes in the virtual machine must be subclassed by one of the
>>   following allocation classes.
>> 
>> Better would be to separate out the present implementation of
>> GCTraceTime into a helper class (let's call it GCTraceTimeImpl), which
>> is a VALUE_OBJ_CLASS_SPEC class. Then GCTraceTime, GCCMTraceTime, and
>> GCRefTraceTime all derive from StackObj and have a GCTraceTimeImpl
>> member.
> 
> Good suggestion.
> 
> Here's an updated webrev:
> http://cr.openjdk.java.net/~brutisso/8136991/webrev.05
> 
> (Note that referenceType.hpp is unchanged even if webrev decides to include it.)
> 
> And here's a diff compared to the last version:
> http://cr.openjdk.java.net/~brutisso/8136991/webrev.04-05.diff

Looks good.