<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Nice cleanup Bengt.<div><br></div><div>One observation which I found a bit confusing.</div><div><br></div><div>In g1CollectorPolicy.cpp, line 2285 - 2287 we have the following code:</div><div><span class="Apple-style-span" style="-webkit-border-horizontal-spacing: 2px; -webkit-border-vertical-spacing: 2px; font-family: Times; "><pre>_last_gc_was_young = gcs_are_young() ? true : false;</pre></span><span class="Apple-style-span" style="-webkit-border-horizontal-spacing: 2px; -webkit-border-vertical-spacing: 2px; font-family: Times; "><pre><span class="new" style="color: blue; font-weight: bold; ">_trace_gen0_time_data.increment_collection_count(!_last_gc_was_young);</span></pre></span><div>The !_last_gc_was_young threw me a bit since we were incrementing the collection count on a "trace gen0". Has there been consideration of using an enumerated type for a young GC and mixed GC type rather than using a boolean to represent the two? Or, alternatively introducing a mutator method for incrementing only _young_pause_num and a second mutator method for incrementing _mixed_pause_num ? </div></div><div><br></div><div>I didn't see anything incorrect in the implementation ... just found this bit a little confusing.</div><div><br></div><div>hths,</div><div><br></div><div>charlie ...</div><div><br><div><div>On May 29, 2012, at 1:58 AM, Bengt Rutisson wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>Hi again,<br><br>I should also have mentioned that this change should not alter the log <br>output. (Except for the ParallelGCThreads = 0 case which now will look <br>the same as PrallelGCThreads >= 1 case.)<br><br>I tested this by leaving the old code in and comparing the output from <br>the old code with the new code. In my tests they look exactly the same.<br><br>Bengt<br><br>On 2012-05-28 22:37, Bengt Rutisson wrote:<br><blockquote type="cite"><br></blockquote><blockquote type="cite">Hi all,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Can I have a couple of code reviews for this change?<br></blockquote><blockquote type="cite"><a href="http://cr.openjdk.java.net/~brutisso/7172279/webrev.01/">http://cr.openjdk.java.net/~brutisso/7172279/webrev.01/</a><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Background<br></blockquote><blockquote type="cite">I am preparing a change to remove the special treatment of the single <br></blockquote><blockquote type="cite">threaded case for G1 PrintGCDetails output. We should be able to use <br></blockquote><blockquote type="cite">the same code for both ParallelGCThreads = 0 and PrallelGCThreads >= <br></blockquote><blockquote type="cite">1. This will hopefully simplify the logging code.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">In preparation for that change I would like to clean up the <br></blockquote><blockquote type="cite">TraceGen0Time and TraceGen1Time data gathering code a bit. It is <br></blockquote><blockquote type="cite">currently kind of difficult to follow the code and there is no clear <br></blockquote><blockquote type="cite">distinction between the data gathered for these two cases compared to <br></blockquote><blockquote type="cite">the data gathered for PrintGCDetails output.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Here is what I tried to do:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- Remove the unnecessary define_num_seq macro and the multiple <br></blockquote><blockquote type="cite">inheritance of the Summary class.<br></blockquote><blockquote type="cite">- Replaced the above with two classes, TraceGen0TimeData and <br></blockquote><blockquote type="cite">TraceGen1TimeData, that will encapsulate the data needed.<br></blockquote><blockquote type="cite">- Made sure that the data is only updated if the corresponding flags <br></blockquote><blockquote type="cite">are turned on.<br></blockquote><blockquote type="cite">- Removed the separate "non-parallel" case. All data is available even <br></blockquote><blockquote type="cite">with ParallelGCThreads = 0.<br></blockquote><blockquote type="cite">- Removed the ExitAfterGCNum flag. I hardly think this is an <br></blockquote><blockquote type="cite">appropriate product flag to have in the code.<br></blockquote><blockquote type="cite">- Removed some unused methods.<br></blockquote><blockquote type="cite">- Removed 7 year old assert(true) with accompanying comments.<br></blockquote><blockquote type="cite">- Removed the unused aux time concept.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">There was one thing that I was hesitant to remove, but finally decided <br></blockquote><blockquote type="cite">to remove anyway. It was the G1CollectorPolicy::check_other_times() <br></blockquote><blockquote type="cite">method. First I added the same method to TraceGen0TimeData. But it is <br></blockquote><blockquote type="cite">a lot of code and I am not sure it is very useful. It can't do any <br></blockquote><blockquote type="cite">exact checks and it seems unlikely to me that it will detect any <br></blockquote><blockquote type="cite">issues. Also, it seems to me like the code was in the wrong place. I <br></blockquote><blockquote type="cite">would have preferred it in some kind of verify method rather than in a <br></blockquote><blockquote type="cite">print method.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Thanks,<br></blockquote><blockquote type="cite">Bengt<br></blockquote><br></div></blockquote></div><br></div></body></html>