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

[Bengt Rutisson]

Hi John,

On 5/3/13 7:51 PM, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking at the backports. I really appreciate it.

No problem. Sorry for missing the request for 8005032 and 8009536 
earlier. Reviewed it now.

Thanks,
Bengt

>
> 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/20130505/e5c29b75/attachment.htm>