Request for review (M): 7178363 G1: Remove the serial code for PrintGCDetails and make it a special case of the parallel code

[Vitaly Davidovich]

Hi Bengt,

A few minor comments.

In g1CollectedHeap.cpp:

1) probably doesn't matter much in its current form, but would it be better
to call os::elapsedTime() right before prepare_verify() so that the timing
info captures just the verification time and not printing to tty/gclog and
constructing the HandleMark? This is in verify().

2) it seems a bit weird to have the "guard" parameter in the verify()
method.  Why wouldn't caller just check the guard and then only call if it
passes? Not a big deal though, just stylistic.

3) in verify_before/after(), would it make sense to only record the time if
it's not 0.0? I'm thinking that the pointer chasing may possible get a
cache miss only to record a 0.  Again, pure speculation.

4) is os::elapsedTime() using a monotonic time source? If not, what would
happen if you get a negative value in the timing?

Looks good otherwise.

Thanks

Sent from my phone
On Aug 22, 2012 7:21 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com> wrote:

>
> Hi again,
>
> Here is an updated webrev based on comments from John Cuthbertson:
> http://cr.openjdk.java.net/~**brutisso/7178363/webrev.02/<http://cr.openjdk.java.net/~brutisso/7178363/webrev.02/>
>
> Here is a diff compared to the previous webrev:
> http://cr.openjdk.java.net/~**brutisso/7178363/webrev.01-02-**diff/<http://cr.openjdk.java.net/~brutisso/7178363/webrev.01-02-diff/>
>
> Thanks,
> Bengt
>
> On 2012-07-20 14:17, Bengt Rutisson wrote:
>
>>
>> Hi all,
>>
>> Here is an updated webrev for this change:
>> http://cr.openjdk.java.net/~**brutisso/7178363/webrev.01/<http://cr.openjdk.java.net/~brutisso/7178363/webrev.01/>
>>
>> It turns out the the earlier change for 7178361 had introduced two more
>> issues: the heap transition information for the PrintGC output and the
>> output for evacuation failures had both been moved in an unintended way.
>> The above webrev corrects both of these chagne too. Thanks John Cuthbertson
>> for pointing me to the evacuation failure output.
>>
>> Bengt
>>
>>
>> On 2012-07-19 11:01, Bengt Rutisson wrote:
>>
>>>
>>> Hi again,
>>>
>>> Just in case anybody already started looking at this: I have updated the
>>> webrev since I had to make some changes to make it compile with the NMT
>>> fixes that have just made it into the hotspot-gc repository. Those updates
>>> were just to make it compile and not really related to my change, so I just
>>> overwrote the existing webrev. Just use the same link as before:
>>>
>>> http://cr.openjdk.java.net/~**brutisso/7178363/webrev.00/<http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/>
>>>
>>> Also, if you want to see what the new output looks like I am attaching a
>>> file called new.txt with an example from running SpecJBB2005 with this
>>> command line:
>>>
>>> -XX:+UseG1GC -XX:ParallelGCThreads=4 -XX:+**UnlockExperimentalVMOptions
>>> -XX:G1LogLevel=finest -XX:+TraceGen0Time -Xms256m -Xmx2G
>>>
>>> I am also attaching a file called old.txt with what the output, using
>>> the same command line, looked like before my change. As you can see the
>>> differences are what I listed in my earlier email. You will also notice
>>> that the "old" version has an entry for the SATB filtering, even though all
>>> the entries are 0 (we didn't do a concurrent cycle so there has been no
>>> SATB filtering). This was a bug I just introduced with my last change (for
>>> 7178361), so the new example is more correct.
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 2012-07-18 15:55, Bengt Rutisson wrote:
>>>
>>>>
>>>> Hi everyone,
>>>>
>>>> I would like some reviews of this change:
>>>> http://cr.openjdk.java.net/~**brutisso/7178363/webrev.00/<http://cr.openjdk.java.net/~brutisso/7178363/webrev.00/>
>>>>
>>>> This removes the special treatment for ParallelGCThreads=0 from the G1
>>>> logging. I did keep the log output unchanged for that case. Basically it
>>>> just has one indentation level less and skips some output. I am not sure
>>>> this is really necessary since it is really a special case. I'm open to
>>>> change that special treatment too and just have the same output as for
>>>> ParallelGCThreads=1.
>>>>
>>>> The PrintGCDetails log output should be the same as before with three
>>>> minor adjustments:
>>>>
>>>> - The "Sum" is now not printed for the start and end values for GC
>>>> workers. This sum does not really make sense to me.
>>>>
>>>> - The "(ms)" unit was removed from output that aren't in milliseconds
>>>> (termination attempts for example).
>>>>
>>>> - The average value is now printed as a double for all types.
>>>>
>>>> I tried to clean up the code a bit and introduced a separate class,
>>>> Snippet WorkerDataArray, to keep track of the per thread logging. I also
>>>> introduced getters and setters to avoid having to make G1CollectorPolicy
>>>> and TraceGen0TimeData friend classes to G1GCPhaseTimes.
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120822/30d8e424/attachment.htm>