RFR(XS): JDK-8129440 G1 crash during concurrent root region scan.

[Jon Masamitsu]

On 07/10/2015 12:38 AM, Siebenborn, Axel wrote:
>
> On 09.07.2015 at 18:50 Jon Masamitsu wrote:
>> On 7/9/2015 1:29 AM, Siebenborn, Axel wrote:
>>> On 06.07.2015 at 15:03 Volker Simonis wrote:
>>>> On Tue, Jun 23, 2015 at 8:45 AM, Siebenborn, Axel
>>>> <axel.siebenborn at sap.com> wrote:
>>>>> Hi Volker,
>>>>>
>>>>> thanks for your comment.Indeed, this was my first idea for a fix. However,
>>>>> there are more than 50 callers of  load_heap_oop, plus some generated by
>>>>> preprocessor defines like this one:
>>>>>
>>>>> #define DO_OOP_WORK_IMPL(closureName) \
>>>>>     template <class T> inline void closureName##Closure::do_oop_work(T* p) {
>>>>> \
>>>>>       T heap_oop = oopDesc::load_heap_oop(p);               \
>>>>>       if (!oopDesc::is_null(heap_oop)) {                    \
>>>>>         oop obj = oopDesc::decode_heap_oop_not_null(heap_oop);       \
>>>>>         do_oop(obj);                                        \
>>>>>       }                                                     \
>>>>>     }
>>>>>
>>>>> We would have to decide for each of the callers, if there might be a
>>>>> concurrency problem or not.
>>>>>
>>>>> Do you think, it is worth the effort and the risk of missing one?
>>>> OK, you're probably right.
>>>>
>>>> If you haven’t seen any performance regressions caused by this fix I'm
>>>> fine with your solution.
>>>>
>>>> Unfortunately I can not push this shared-code change so can we please
>>>> get a sponsor from within Oracle for this change?
>>>>
>>>> Thanks,
>>>> Volker
>>> We did not see any performance regressions.
>>> On Itanium compilers add release and acquire semantics to stores and loads of volatile variables. The additional memory barriers really could have a performance impact. But even on Itanium we don’t see any performance regression in our benchmarks.
>>> On other platforms, optimizations prevented by the volatile qualifier are optimizations that we really don't want.
>> Did you do performance measurements on GC pause times?
>>
>> Jon
> I checked the GC pause times in standard benchmarks for ParrallelGC, CMS and G1. I don't see regressions here.

Great.  Can you send me the GC log files.  Probably just to me is OK 
unless someone
else want to look at them.

Jon

>
> Axel
>
>>>   
>>> Thanks,
>>> Axel
>>>>> Regards,
>>>>>
>>>>> Axel
>>>>>
>>>>> On 22.06.2015 at 18:17 Volker Simonis wrote:
>>>>>> Hi Axel,
>>>>>>
>>>>>> the change looks good, but shouldn't we better provide two versions of
>>>>>> load_heap_oop()
>>>>>>
>>>>>> inline oop       oopDesc::load_heap_oop(oop* p)
>>>>>> inline oop       oopDesc::load_heap_oop(volatile oop* p)
>>>>>>
>>>>>> such that the callers can decide on their own which versions they require?
>>>>>>
>>>>>> Or do the callers of load_heap_oop() not always know if there exist
>>>>>> concurrent mutator threads?
>>>>>>
>>>>>> Regards,
>>>>>> Volker
>>>>>>
>>>>>>
>>>>>> On Mon, Jun 22, 2015 at 5:33 PM, Siebenborn, Axel
>>>>>> <axel.siebenborn at sap.com> wrote:
>>>>>>> Hi,
>>>>>>> we have seen crashes with G1 during concurrent root region scan.
>>>>>>> It turned out, that the reason for the crashes was reloading an oop after
>>>>>>> a null check.
>>>>>>> The compiler may legally do this, as the pointer is not declared as
>>>>>>> volatile.
>>>>>>> The code runs concurrently to mutator threads.
>>>>>>>
>>>>>>> template <class T>
>>>>>>> inline void G1RootRegionScanClosure::do_oop_nv(T* p) {
>>>>>>>     T heap_oop = oopDesc::load_heap_oop(p);
>>>>>>> // 1. Load oop
>>>>>>>     if (!oopDesc::is_null(heap_oop)) {
>>>>>>>       oop obj = oopDesc::decode_heap_oop_not_null(heap_oop);    // 2.
>>>>>>> Compiler decides to re-load the oop
>>>>>>>       HeapRegion* hr = _g1h->heap_region_containing((HeapWord*) obj);
>>>>>>>       _cm->grayRoot(obj, obj->size(), _worker_id, hr);
>>>>>>>     }
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> We have seen the problem on AIX with the xlC compiler. However, we have
>>>>>>> seen similar optimizations on other platforms and other platforms.
>>>>>>>
>>>>>>> As this code pattern is used quite often, I would suggest a global fix in
>>>>>>> oopDesc::load_heap_oop(p).
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8129440
>>>>>>> Webrev: http://cr.openjdk.java.net/~asiebenborn/8129440/webrev.00/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Axel