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

[Bengt Rutisson]

Thanks for the review, Kim!

Bengt

On 2015-09-25 20:56, Kim Barrett wrote:
> 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:nks
>>> 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.
>