RFR (M): 8145435: [JVMCI] some tests on Windows fail with: assert(!thread->is_Java_thread()) failed: must not be java thread
David Holmes
david.holmes at oracle.com
Tue Dec 22 12:16:03 UTC 2015
Hi Christian,
I'm abstaining from further comment on this.
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