RFR(M): Backports of G1 bugs 8009940, 8008301, 8010463, 8010780, 8012335, and 8012715 to hs24

[John Cuthbertson]

Hi Bengt,

Thanks for looking at the backports. I really appreciate it.

On 5/2/2013 2:27 PM, Bengt Rutisson wrote:
>
> Hi John,
>
> Thanks for providing both the old and new webrevs. Makes this much 
> easier to review!
>
> Comments inline...
>
> On 5/2/13 9:04 PM, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Some more backports that did not apply cleanly to review:
>>
>> *8009940:****G1: assert(_finger == _heap_end) failed, 
>> concurrentMark.cpp:809*
>> Backport: 
>> http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.2.8009940.hsx24.patch/
>> Original: http://cr.openjdk.java.net/~johnc/8009940/webrev.1/
>>
>> The changes did not apply cleanly because of CMTask::_worker_id being 
>> renamed to CMTask::_task_id.
>
> Looks good.

Thanks.

>
>>
>> Note these changes are generated w.r.t the backports for 8005032 and 
>> 8009536 being applied.
>>
>> *8010463: G1: Crashes with -UseTLAB and heap verification*
>> Backport: 
>> http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/
>> Original: http://cr.openjdk.java.net/~johnc/8010463/webrev.0/
>>
>> I'm not sure why this did not apply cleanly but the rejected chunk 
>> was in g1CollectedHeap.cpp.
>
> In g1CollectedHeap.cpp it did not apply cleanly because of changes for 
> perm gen removal.
>
> I think the original (hs25) webrev may not be up-to-date.
>
> The tests in the webrevs are different:
>
> http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/test/gc/TestVerifyBeforeGCDuringStartup.java.html
> http://cr.openjdk.java.net/~johnc/8010463/webrev.0/test/gc/TestVerifyBeforeGCDuringStartup.java.html
>
> But your backported test looks like the one that was actually pushed 
> to hs25:
>
> http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/file/24ef5fb05e0f/test/gc/TestVerifyBeforeGCDuringStartup.java 
>
>
> Similarly, what you actually pushed to hs25 in thread.cpp looks more 
> like what you propose to backport to hs24:
>
> http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/diff/24ef5fb05e0f/src/share/vm/runtime/thread.cpp
> http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/src/share/vm/runtime/thread.cpp.udiff.html
>
> So, I think this looks good.

Thanks. I must not have updated the original webrev after applying your 
code review comments so supplying the link to the original was probably 
not useful in this case.


>
>>
>> *8010780: G1: Eden occupancy/capacity output wrong after a full GC*
>> Backport: 
>> http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.5.8010780.hsx24.patch/
>> Original: http://cr.openjdk.java.net/~johnc/8010780/webrev.0/
>>
>> The changes (especially in g1CollectedHeap.cpp) conflicted with the 
>> tracing changes.
>
> A bit difficult to review since the patch files are quite different. 
> As far as I can tell it looks good.

Thanks. The additional indentation is probably throwing the stats off. 
There's more code in G1CollectedHeap::do_collection() for the tracing 
and I had to indent that additional code, causing an increase in the 
number of lines modified. I was more concerned that I had preserved 
relative order w.r.t. the tracing code.

Could I bother you to review the backports for 8005032 and 8009536? They 
underpin a couple of the changes above.

Thanks again,

JohnC

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130503/dba21ba1/attachment.htm>