RFR (XS): 8022595: JSR292: deadlock during class loading of MethodHandles, MethodHandleImpl & MethodHandleNatives

[David Holmes]

On 21/08/2013 8:35 AM, Coleen Phillimore wrote:
>
> On 08/20/2013 06:24 PM, Coleen Phillimore wrote:
>>
>>
>> Vladimir,
>>
>> You are right, it's broken.  Calvin is working on a similar issue in
>> bug https://jbs.oracle.com/bugs/browse/JDK-8020675.
>
> Reading more, I think Calvin's issue is similar but different.  I will
> file a new bug for the Threads::create_vm() EXCEPTION_MARK.

I think the intent is that we never, ever expect to get any exceptions 
during VM initialization unless we have a significant bug. The 
EXCEPTION_MARK detects that bug in debug builds.

I have a vague recollection of the CHECK_0 "bug" being discussed when I 
was in runtime.

David

> Coleen
>
>>
>> I think you should check in your code and we'll fix all the
>> CHECK_0/EXCEPTION_MARK problems at once.
>>
>> Thanks for checking this out.
>> Coleen
>>
>> On 08/20/2013 05:55 PM, Vladimir Ivanov wrote:
>>> Coleen,
>>>
>>> Thank you for the review.
>>> It doesn't cause a deadlock anymore because class loading is
>>> guaranteed to be performed in singe-threaded mode.
>>>
>>> Regarding CHECK_0 -> CHECK_(JNI_ERR) change you propose, there's one
>>> problem. All pending exceptions are fatal right now: even if it
>>> returns JNI_OK, there's an ExceptionMark on the stack which crashes
>>> VM if any pending exceptions are present.
>>>
>>> Considering that JNI_CreateJavaVM uses Thread::create_vm and failure
>>> during VM initialization can crash the whole app, I don't think such
>>> behavior is correct.
>>>
>>> So, I have 2 questions here:
>>>     1) what behavior do we prefer?
>>>     2) if we want to change current behavior, doesn't it deserve a
>>> separate fix?
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 8/20/13 11:02 PM, Coleen Phillimore wrote:
>>>>
>>>> Vladimir,
>>>>
>>>> The code that you added is copied from the initialization code
>>>> above.  I
>>>> think they have a problem.  If initialization fails, there is a CHECK_0
>>>> at the end of the call.  CHECK_0 returns JNI_OK to the caller, which
>>>> goes ahead with the initialization.
>>>>
>>>> Can you change all of these to CHECK_(JNI_ERR) so that the caller can
>>>> deal with the error?  It would be good to see if the caller does the
>>>> right thing with a JNI_ERR.
>>>>
>>>> Otherwise, I think the code is fine if the initialization of these
>>>> classes doesn't cause deadlocks in their new place.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 08/20/2013 01:05 PM, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/8022595/webrev.01/
>>>>> 201 lines changed: 201 ins; 0 del; 0 mod;
>>>>>
>>>>> Some classes in j.l.invoke have circular dependencies between them
>>>>> which leads to deadlocks during class initialization when multiple
>>>>> threads exercise JSR292 functionality.
>>>>>
>>>>> JSR292 core classes are among "well-known" classes to VM and they are
>>>>> preloaded during VM startup (see
>>>>> SystemDictionary::initialize_preloaded_classes). However, it doesn't
>>>>> force preloaded classes to be initialized (doesn't execute static
>>>>> initializers). Thus, these classes should be explicitly
>>>>> pre-initialized in order to avoid deadlocks.
>>>>>
>>>>> Based on my observations of possible deadlock scenarios, I chose 3
>>>>> classes to pre-initialize:
>>>>>     - MethodHandle
>>>>>     - MemberName
>>>>>     - MethodHandleNatives
>>>>>
>>>>> I placed the code right after compilers initialization because during
>>>>> method resolution for signature polymorphic intrinsics, compilation
>>>>> tasks are issued (see SystemDictionary::find_method_handle_intrinsic
>>>>> for details) and they are missed if compiler subsystem isn't ready
>>>>> yet.
>>>>>
>>>>> Testing: failing test, regression test, JPRT, jdk/test/j/l/invoke,
>>>>> vm.mlvm.testlist, octane.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>
>>
>