RFR: 8139424: SIGSEGV, Problematic frame: # V [libjvm.so+0xd0c0cc] void InstanceKlass::oop_oop_iterate_oop_maps_specialized<true, oopDesc*, MarkAndPushClosure>

[Stefan Johansson]

On 2015-11-12 17:42, Kim Barrett wrote:
> On Nov 12, 2015, at 11:25 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>>
>> On 2015-11-12 15:53, Kim Barrett wrote:
>>> On Nov 12, 2015, at 3:47 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>>>> On 2015-11-12 01:23, Kim Barrett wrote:
>>>>> On Nov 11, 2015, at 10:41 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this fix for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8139424
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sjohanss/8139424/hotspot.00/
>>>>> Good find!
>>>>>
>>>>> The added guarantee in RegisterHumongousWithInCSetFastTestClosure::doHeapRegion looks like leftover debugging code.
>>>> Thanks for looking at this Kim,
>>>>
>>>> It is from the debugging, but it was left on purpose. This guarantee will help us catch the case where we iterated the old version of the SparsePRT and not the expanded one, because the iterator uses SparsePRT->_cur to iterate while occupied uses SparsePRT->_next. I could update the message to say:
>>>> "Remembered set hash maps out of sync, cur: " SIZE_FORMAT " entries, next: " SIZE_FORMAT " entries"
>>>>
>>>> I think this is more informative and I prefer to leave the guarantee in. Updated webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8139424/hotspot.01/
>>> It seems like this check might be more appropriately placed in the constructor for
>>> HeapRegionRemSetIterator, for the benefit of anyone iterating over a remset.
>>> Although that might make HeapRegionRemSet’s usage a problem.
>>>
>> In the constructor we can't know how many entries were actually iterated, n_yielded is accounted during the iteration. In this case, what I want to assert is that we iterate as many entires as we expect. The other use of the iterator in ScanRSClosure might not have the same constraints so for now I would prefer to leave the assert where it is (note that I changed it to an assert).
> The real check we want is that we’re iterating over an up-to-date set.  The use of n_yielded is just
> an external mechanism for determining that.  The iterator constructor could, I think, directly check
> whether the set is up to date.
>
> Given where ScanRSClosure is used (ultimately, in G1CollectedHeap::evacuate_collection_set),
> it is already implicitly in the appropriate context, and it seems likely that missing entries here
> because a set is not up-to-date would be quite bad.
>
> However, in the interest of getting the crashes fixed, I’d be ok with going ahead with webrev.02
> and a followup RFE.
>
Thanks Kim,

I'll try to get this pushed tomorrow and I'll file follow up RFEs for 
the things that have been mentioned in the revivew.

Stefan