[9] RFR(S): 8029443: 'assert(klass->is_loader_alive(_is_alive)) failed: must be alive' during VM_CollectForMetadataAllocation

[Vladimir Kozlov]

On 7/18/14 11:02 AM, Coleen Phillimore wrote:
>
> On 7/18/14, 11:06 AM, Vladimir Kozlov wrote:
>> On 7/18/14 4:38 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> I spend some more days and was finally able to implement a test that
>>> deterministically triggers the bug:
>>
>> Why do you need to switch off compressed oops? Do you need to switch
>> off compressed klass pointers too (UseCompressedClassPointers)?
>
> CompressedOops when off turns off CompressedClassPointers.

You are right, I forgot that. Still the question is why switch off coop?

Vladimir

>>
>>>
>>> http://cr.openjdk.java.net/~thartmann/8029443/webrev.02/
>>
>> Very nice!
>
> Yes, I agree.  Impressive.
>
> The refactoring in nmethod.cpp looks good to me.  I have no further
> comments.
> Thanks!
> Coleen
>
>>
>>>
>>> @Vladimir: The test shows why we should only clean the ICs but not
>>> unload the nmethod if possible. The method ' doWork'
>>> is still valid after WorkerClass was unloaded and depending on the
>>> complexity of the method we should avoid unloading it.
>>
>> Make sense.
>>
>>>
>>> On Sparc my patch fixes the bug and leads to the nmethod not being
>>> unloaded. The compiled version is therefore used even
>>> after WorkerClass is unloaded.
>>>
>>> On x86 the nmethod is unloaded anyway because of a dead oop. This is
>>> probably due to a slightly different implementation
>>> of the ICs. I'll have a closer look to see if we can improve that.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> Tobias
>>>
>>> On 16.07.2014 10:36, Tobias Hartmann wrote:
>>>> Sorry, forgot to answer this question:
>>>>> Were you able to create a small test case for it that would be
>>>>> useful to add?
>>>> Unfortunately I was not able to create a test. The bug only
>>>> reproduces on a particular system with a > 30 minute run
>>>> of runThese.
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>> On 16.07.2014 09:54, Tobias Hartmann wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> thanks for the review.
>>>>>> *+           if (csc->is_call_to_interpreted() &&
>>>>>> stub_contains_dead_metadata(is_alive, csc->destination())) {*
>>>>>> *+             csc->set_to_clean();*
>>>>>> *+           }*
>>>>>>
>>>>>> This appears in each case.  Can you fold it and the new function
>>>>>> into a function like
>>>>>> clean_call_to_interpreted_stub(is_alive, csc)?
>>>>>
>>>>> I folded it into the function clean_call_to_interpreter_stub(..).
>>>>>
>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8029443/webrev.01/
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> So before the permgen removal embedded method* were oops and they
>>>>>>> were processed in relocInfo::oop_type loop.
>>>>>>>
>>>>>>> May be instead of specializing opt_virtual_call_type and
>>>>>>> static_call_type call site you can simple add a loop for
>>>>>>> relocInfo::metadata_type (similar to oop_type loop)?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 7/14/14 4:56 AM, Tobias Hartmann wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> please review the following patch for JDK-8029443.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8029443
>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8029443/webrev.00/
>>>>>>>>
>>>>>>>> *Problem*
>>>>>>>> After the tracing/marking phase of GC, nmethod::do_unloading(..)
>>>>>>>> checks
>>>>>>>> if a nmethod can be unloaded because it contains dead oops. If
>>>>>>>> class
>>>>>>>> unloading occurred we additionally clear all ICs where the cached
>>>>>>>> metadata refers to an unloaded klass or method. If the nmethod
>>>>>>>> is not
>>>>>>>> unloaded, nmethod::verify_metadata_loaders(..) finally checks if
>>>>>>>> all
>>>>>>>> metadata is alive. The assert in CheckClass::check_class fails
>>>>>>>> because
>>>>>>>> the nmethod contains Method* metadata corresponding to a dead
>>>>>>>> Klass.
>>>>>>>> The Method* belongs to a to-interpreter stub [1] of an optimized
>>>>>>>> compiled IC. Normally we clear those stubs prior to verification to
>>>>>>>> avoid dangling references to Method* [2], but only if the stub
>>>>>>>> is not in
>>>>>>>> use, i.e. if the IC is not in to-interpreted mode. In this case the
>>>>>>>> to-interpreter stub may be executed and hand a stale Method* to the
>>>>>>>> interpreter.
>>>>>>>>
>>>>>>>> *Solution
>>>>>>>> *The implementation of nmethod::do_unloading(..) is changed to
>>>>>>>> clean
>>>>>>>> compiled ICs and compiled static calls if they call into a
>>>>>>>> to-interpreter stub that references dead Method* metadata.
>>>>>>>>
>>>>>>>> The patch was affected by the G1 class unloading changes
>>>>>>>> (JDK-8048248)
>>>>>>>> because the method nmethod::do_unloading_parallel(..) was added. I
>>>>>>>> adapted the implementation as well.
>>>>>>>> *
>>>>>>>> Testing
>>>>>>>> *Failing test (runThese)
>>>>>>>> JPRT
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Tobias
>>>>>>>>
>>>>>>>> [1] see CompiledStaticCall::emit_to_interp_stub(..)
>>>>>>>> [2] see nmethod::verify_metadata_loaders(..),
>>>>>>>> static_stub_reloc()->clear_inline_cache() clears the stub
>>>>>>
>>>>>
>>>>
>>>
>