RFR (M): 8145435: [JVMCI] some tests on Windows fail with: assert(!thread->is_Java_thread()) failed: must not be java thread

Christian Thalinger christian.thalinger at oracle.com
Tue Dec 22 18:35:19 UTC 2015 

> On Dec 22, 2015, at 2:16 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Christian,
> 
> I'm abstaining from further comment on this.

Fair enough.  Thanks for your input.

> 
> Cheers,
> David
> 
> On 22/12/2015 2:25 AM, Christian Thalinger wrote:
>> 
>>> On Dec 17, 2015, at 9:33 PM, Christian Thalinger
>>> <christian.thalinger at oracle.com
>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>> 
>>>> 
>>>> On Dec 17, 2015, at 6:37 PM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>> 
>>>> On 18/12/2015 3:29 AM, Christian Thalinger wrote:
>>>>> 
>>>>>> On Dec 16, 2015, at 10:12 PM, David Holmes <david.holmes at oracle.com
>>>>>> <mailto:david.holmes at oracle.com><mailto:david.holmes at oracle.com>
>>>>>> <mailto:david.holmes at oracle.com<mailto:david.holmes at oracle.com>>>
>>>>>> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 17/12/2015 3:45 AM, Christian Thalinger wrote:
>>>>>>> 
>>>>>>>> On Dec 15, 2015, at 2:54 PM, David Holmes
>>>>>>>> <david.holmes at oracle.com
>>>>>>>> <mailto:david.holmes at oracle.com><mailto:david.holmes at oracle.com>
>>>>>>>> <mailto:david.holmes at oracle.com<mailto:david.holmes at oracle.com>>>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> Hi Christian,
>>>>>>>> 
>>>>>>>> os_posix.cpp:
>>>>>>>> 
>>>>>>>> tl;dr: the new assert is wrong, don't add it.
>>>>>>>> 
>>>>>>>> os::sleep has two paths:
>>>>>>>> 
>>>>>>>> 1. interruptible sleeps
>>>>>>>> 
>>>>>>>> This is the implementation of java.lang.Thread.sleep and should only
>>>>>>>> be called by JavaThreads using that API (but see below)
>>>>>>>> 
>>>>>>>> 2. non-interruptible sleep
>>>>>>>> 
>>>>>>>> Used for 'incidental' sleeps within the VM, primarily by
>>>>>>>> non-JavaThreads. If a JavaThread uses this it will delay safepoints,
>>>>>>>> so the sleep should be very small.
>>>>>>>> 
>>>>>>>> As you have discovered the Windows implementation of os::sleep
>>>>>>>> actually asserts that JavaThreads don't take the second path. That's
>>>>>>>> not correct. There are two existing cases where this will trigger
>>>>>>>> today.
>>>>>>>> 
>>>>>>>> The first is a historical usage of ConvertYieldToSleep:
>>>>>>>> 
>>>>>>>> JVM_ENTRY(void, JVM_Yield(JNIEnv *env, jclass threadClass))
>>>>>>>> JVMWrapper("JVM_Yield");
>>>>>>>> if (os::dont_yield()) return;
>>>>>>>> HOTSPOT_THREAD_YIELD();
>>>>>>>> 
>>>>>>>> // When ConvertYieldToSleep is off (default), this matches the
>>>>>>>> classic VM use of yield.
>>>>>>>> // Critical for similar threading behaviour
>>>>>>>> if (ConvertYieldToSleep) {
>>>>>>>> os::sleep(thread, MinSleepInterval, false);
>>>>>>>> } else {
>>>>>>>> os::naked_yield();
>>>>>>>> }
>>>>>>>> JVM_END
>>>>>>>> 
>>>>>>>> Then in JVM_Sleep itself:
>>>>>>>> 
>>>>>>>> if (millis == 0) {
>>>>>>>> if (ConvertSleepToYield) {
>>>>>>>>   os::naked_yield();
>>>>>>>> } else {
>>>>>>>>   ThreadState old_state = thread->osthread()->get_state();
>>>>>>>>   thread->osthread()->set_state(SLEEPING);
>>>>>>>>   os::sleep(thread, MinSleepInterval, false);
>>>>>>>>   thread->osthread()->set_state(old_state);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> This would also blow up on Windows - if ConvertSleepToYield is
>>>>>>>> turned off (it is on by default on x86 - in fact all platforms
>>>>>>>> except sparc - which really should be a Solaris setting as it will
>>>>>>>> now affect the Linux/sparc port (which would then crash with your
>>>>>>>> proposed additional assert in os_posix.cpp!).
>>>>>>>> 
>>>>>>>> So adding the assert to os::sleep on Posix is not correct, and
>>>>>>>> arguably the assertion should be removed from the windows code.
>>>>>>> 
>>>>>>> I’ll let you guys take care of that.
>>>>>>> 
>>>>>>>> 
>>>>>>>> --
>>>>>>>> 
>>>>>>>> os_windows.cpp:
>>>>>>>> runtime/os.hpp"
>>>>>>>> 
>>>>>>>> If you grep you will find about a 50-50 split between interruptible
>>>>>>>> and interruptable through the codebase, so unless you want to file a
>>>>>>>> cleanup bug to make it consistent I would just drop this change.
>>>>>>> 
>>>>>>> It’s a cleanup and it’s already done but I’m not arguing for this.
>>>>>>> 
>>>>>>>> 
>>>>>>>> ---
>>>>>>>> 
>>>>>>>> java.cpp
>>>>>>>> 
>>>>>>>> It is not at all clear to me will be in a position to print the
>>>>>>>> exception information correctly at this stage of the termination
>>>>>>>> logic. I would argue that any errors during JVMCIRuntime::shutdown
>>>>>>>> should be handled internally and logged using the logging mechanism,
>>>>>>>> not via exceptions and exception printing.
>>>>>>> 
>>>>>>> I disagree.  We are shutting down the VM and while doing that we are
>>>>>>> running Java code.  If the Java code throws an exception the user
>>>>>>> wants to see that exception.
>>>>>> 
>>>>>> But it is not the user's Java code it is an internal implementation
>>>>>> artifact. I have no doubt this is helping you with debugging JVMCI but
>>>>>> I don't think it is the correct way to handle an internal JVMCI error
>>>>>> during termination. Ditto for the use of the sleep mechanism.
>>>>> 
>>>>> I’m not sure I understand.  Are you saying that users should not see
>>>>> exceptions that are thrown from system code when the VM starts up and
>>>>> shuts down?  That doesn’t make any sense.
>>>> 
>>>> I think logging plus use of fatal error handler is preferable. I'm
>>>> genuinely concerned that the VM may not be in any position to print
>>>> exception stacktraces at late stages of VM termination.
>>> 
>>> It is.  Perhaps I should have mentioned that I tried to throw an
>>> exception and it works perfectly fine.  All this is very theoretical
>>> because in practice this will never happen.
>> 
>> Are you ok with the change as it is?
>> 
>>> 
>>>> 
>>>> David
>>>> -----
>>>> 
>>>>> Also, remember that JVMCI is an experimental feature in 9.  It is not
>>>>> turned on by default.
>>>>> 
>>>>>> 
>>>>>> David
>>>>>> ------
>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>> ---
>>>>>>>> 
>>>>>>>> src/share/vm/jvmci/jvmciCompiler.cpp
>>>>>>>> 
>>>>>>>> typo:
>>>>>>>> 
>>>>>>>> +   // Give other aborting threads to also print their stack traces.
>>>>>>>> 
>>>>>>>> should be:
>>>>>>>> 
>>>>>>>> +   // Give other aborting threads a chance to also print their
>>>>>>>> stack traces.
>>>>>>>> 
>>>>>>>> though I am dubious about the need for this and the use of the
>>>>>>>> os::sleep that seemed to trigger this whole issue. Why aren't you
>>>>>>>> using a normal vmError-based termination path for such fatal errors?
>>>>>>> 
>>>>>>> As the comment says:
>>>>>>> 
>>>>>>> +  // This can be very useful when debugging class initialization
>>>>>>> +  // failures.
>>>>>>> 
>>>>>>> Since multiple compiler threads can initialize the JVMCI subsystem we
>>>>>>> need to print all of the exceptions to see the root cause.  Doug
>>>>>>> would know more about this but that’s the gist.
>>>>>>> 
>>>>>>>> 
>>>>>>>> Cheers,
>>>>>>>> David
>>>>>>>> 
>>>>>>>> On 16/12/2015 6:54 AM, Christian Thalinger wrote:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8145435
>>>>>>>>> http://cr.openjdk.java.net/~twisti/8145435/webrev/
>>>>>>>>> 
>>>>>>>>> The actual fix for the problem is passing true to os::sleep:
>>>>>>>>> 
>>>>>>>>> +  const bool interruptible = true;
>>>>>>>>> +  os::sleep(THREAD, 200, interruptible);
>>>>>>>>> 
>>>>>>>>> since compiler threads are Java threads but I fixed a couple more
>>>>>>>>> things...
>>>>>>>>> 
>>>>>>>>> First, I’ve added the same assert to os_posix.cpp:
>>>>>>>>> 
>>>>>>>>> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/os/posix/vm/os_posix.cpp.udiff.html
>>>>>>>>> 
>>>>>>>>> because it’s odd to only hit this assert on Windows:
>>>>>>>>> 
>>>>>>>>> Additionally I’ve changed the way we handle an exception in
>>>>>>>>> before_exit:
>>>>>>>>> 
>>>>>>>>> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/share/vm/runtime/java.cpp.udiff.html
>>>>>>>>> 
>>>>>>>>> and moved abort_on_pending_exception into JVMCICompiler and made it
>>>>>>>>> private. It’s used for one thing only now.
>>>>>>>>> 
>>>>>>>>> Finally I replaced JVMCIRuntime::call_printStackTrace with calls to
>>>>>>>>> java_lang_Throwable::print_stack_trace.
>>

More information about the hotspot-dev mailing list