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

[Bengt Rutisson]

Hi Kim,

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?

>
> ------------------------------------------------------------------------------
> 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.

>
> ------------------------------------------------------------------------------
> 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.

>
> ------------------------------------------------------------------------------
> 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


Thanks,
Bengt


>
> ------------------------------------------------------------------------------
>