RFR: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions
Jamsheed C M
jamsheed.c.m at oracle.com
Thu Sep 3 09:09:12 UTC 2020
Some edits
On 03/09/2020 14:13, Jamsheed C M wrote:
> Hi David,
>
> Thank you for the review and feedback.
>
> Revised webrev here: http://cr.openjdk.java.net/~jcm/8249451/webrev.02/
>
> On 03/09/2020 05:50, David Holmes wrote:
>> Hi Jamsheed,
>>
>> On 1/09/2020 10:36 pm, Jamsheed C M wrote:
>>> Hi David,
>>>
>>> I reworked the patch, revised webrev here:
>>> http://cr.openjdk.java.net/~jcm/8249451/webrev.01/
>>
>> Thanks. The new macros and injected field for InternalError look good.
>>
>> A couple of minor comments below but overall this looks good to me.
>>
>>> In addition I moved UnlockFlagSaver fs(this) to more local scope.
>>>
>>> also removed changes done for JDK-8246727, as it will be separately
>>> handled by the bug.
>>>
>>> Testing: injected and tested async exceptions randomly at
>>> compilation request path and deopt path.
>>
>> I noticed in deoptimization.cpp that here:
>>
>> 1965 load_class_by_index(constants, unloaded_class_index, THREAD);
>>
>> we can now return with a pending async exception and it is unclear
>> whether the code following this will be able to handle that, or
>> indeed whether the caller will be able to handle it. Did you
>> specifically test this site?
>>
> Yes, I browsed through the code path, it is equipped(let it be c2 only
> UC, or various deopt variant).
Below comment is for aot compilation request path. sorry for mixing up
> JVMCI aot code too is equipped to handle it( there are forwarding code
> present at foreign call exit)
>
Best regards,
Jamsheed
>> ---
>>
>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>
>> The comment at:
>>
>> 80 // 1. The pending exception is cleared
>>
>> should be updated now that asyncs are not cleared.
> Done.
>>
>> ---
>>
>> src/hotspot/share/compiler/tieredThresholdPolicy.*
>>
>> The changes from JavaThread* to Thread* look unnecessary for 90% of
>> the cases, but the overall change seems to be dictated by the few
>> methods that do use CHECK*. :( No point agonising over this now as
>> I'm trying to deal with this general problem as a separate RFE -
>> JDK-8252685.
>>
> Thank you and Best regards,
>
> Jamsheed
>
>> Thanks,
>> David
>> -----
>>
>>> Best regards,
>>>
>>> Jamsheed
>>>
>>> On 24/08/2020 11:06, Jamsheed C M wrote:
>>>> Hi David,
>>>>
>>>> Thank you for the review and feedback. Agree on all of them. I will
>>>> rework and get back.
>>>>
>>>> On 10/08/2020 07:33, David Holmes wrote:
>>>>> Hi Jamsheed,
>>>>>
>>>>> On 6/08/2020 10:07 pm, Jamsheed C M wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249451
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~jcm/8249451/webrev.00/
>>>>>
>>>>> Thanks for tackling this messy issue. Overall I like the use of
>>>>> TRAPS to more clearly document which methods can return with an
>>>>> exception pending. I think there are some problems with the
>>>>> proposed changes. I'll start with those comments and then move on
>>>>> to more general comments.
>>>>>
>>>>> src/hotspot/share/utilities/exceptions.cpp
>>>>> src/hotspot/share/utilities/exceptions.hpp
>>>>>
>>>>> I don't think the changes here are correct or safe in general.
>>>>>
>>>>> First, adding the new macro and function to only clear non-async
>>>>> exceptions is fine itself. But naming wise the fact only non-async
>>>>> exceptions are cleared should be evident, and there is no "check"
>>>>> involved (in the sense of the existing CHECK_ macros) so I suggest:
>>>>>
>>>>> s/CHECK_CLEAR_PENDING_EXCEPTION/CLEAR_PENDING_NONASYNC_EXCEPTIONS/
>>>>> s/check_clear_pending_exception/clear_pending_nonasync_exceptions/
>>>>>
>>>> Ok
>>>>> But changing the existing CHECK_AND_CLEAR macros to now leave
>>>>> async exceptions pending seems potentially dangerous as calling
>>>>> code may not be prepared for there to now be a pending exception.
>>>>> For example the use in thread.cpp:
>>>>>
>>>>> JDK_Version::set_runtime_name(get_java_runtime_name(THREAD));
>>>>> JDK_Version::set_runtime_version(get_java_runtime_version(THREAD));
>>>>>
>>>>> get_java_runtime_name() is currently guaranteed to clear all
>>>>> exceptions, so all the other code is known to be safe to call. But
>>>>> that would no longer be true. That said, this is VM initialization
>>>>> code and an async exception is impossible at this stage.
>>>>>
>>>>> I think I would rather see CHECK_AND_CLEAR left as-is, and an
>>>>> actual CHECK_AND_CLEAR_NONASYNC introduced for those users of
>>>>> CHECK_AND_CLEAR that can encounter async exceptions and which
>>>>> should not clear them.
>>>>>
>>>>> + if
>>>>> (!_pending_exception->is_a(SystemDictionary::ThreadDeath_klass()) &&
>>>>> + _pending_exception->klass() !=
>>>>> SystemDictionary::InternalError_klass()) {
>>>>>
>>>> Ok
>>>>> Flagging all InternalErrors as async exceptions is probably also
>>>>> not correct. I don't see a good solution to this at the moment. I
>>>>> think we would need to introduce a new subclass of InternalError
>>>>> for the unsafe access error case**. Now it may be that all the
>>>>> other InternalError usages are "impossible" in the context of
>>>>> where the new macros are to be used, but that is very difficult to
>>>>> establish or assert.
>>>>>
>>>>> ** Or perhaps we could inject a field that allows the VM to
>>>>> identify instances related to unsafe access errors ... Ideally of
>>>>> course these unsafe access errors would be distinct from the async
>>>>> exception mechanism - something I would still like to pursue.
>>>>>
>>>> Ok
>>>>> ---
>>>>>
>>>>> General comments ...
>>>>>
>>>>> There is a general change from "JavaThread* thread" to "Thread*
>>>>> THREAD" (or TRAPS) to allow the use of the CHECK macros. This is
>>>>> unfortunate because the fact the thread is restricted to being a
>>>>> JavaThread is no longer evident in the method signatures. That is
>>>>> a flaw with the TRAPS/CHECK mechanism unfortunately :( . But as
>>>>> the methods no longer take a JavaThread* arg, they should assert
>>>>> that THREAD->is_Java_thread(). I will also look at an RFE to have
>>>>> as_JavaThread() to avoid the need for separate assertion checks
>>>>> before casting from "Thread*" to "JavaThread*".
>>>>>
>>>> Ok
>>>>> Note there's no need to use CHECK when the enclosing method is
>>>>> going to return immediately after the call that contains the
>>>>> CHECK. It just adds unnecessary checking of the exception state.
>>>>> The use of TRAPS shows that the methods may return with an
>>>>> exception pending. I've flagged all such occurrences I spotted below.
>>>>>
>>>> Ok
>>>>> ---
>>>>>
>>>>> + // Only metaspace OOM is expected. no Java code executed.
>>>>>
>>>>> Nit: s/no/No
>>>>>
>>>>>
>>>>> src/hotspot/share/compiler/compilationPolicy.cpp
>>>>>
>>>>>
>>>>> 410 method_invocation_event(method, CHECK_NULL);
>>>>> 489 CompileBroker::compile_method(m, InvocationEntryBci,
>>>>> comp_level, m, hot_count, CompileTask::Reason_InvocationCount,
>>>>> CHECK);
>>>>>
>>>>> Nit: there's no need to use CHECK here.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/compiler/tieredThresholdPolicy.cpp
>>>>>
>>>>> 504 method_invocation_event(method, inlinee, comp_level, nm,
>>>>> CHECK_NULL);
>>>>> 570 compile(mh, bci, CompLevel_simple, CHECK);
>>>>> 581 compile(mh, bci, CompLevel_simple, CHECK);
>>>>> 595 CompileBroker::compile_method(mh, bci, level, mh,
>>>>> hot_count, CompileTask::Reason_Tiered, CHECK);
>>>>> 1062 compile(mh, InvocationEntryBci, next_level, CHECK);
>>>>>
>>>>> Nit: there's no need to use CHECK here.
>>>>>
>>>>> 814 void TieredThresholdPolicy::create_mdo(const methodHandle& mh,
>>>>> Thread* THREAD) {
>>>>>
>>>>> Thank you for correcting this misuse of the THREAD name on a
>>>>> JavaThread* type.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/interpreter/linkResolver.cpp
>>>>>
>>>>> 128 CompilationPolicy::compile_if_required(selected_method, CHECK);
>>>>>
>>>>> Nit: there's no need to use CHECK here.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/jvmci/compilerRuntime.cpp
>>>>>
>>>>> 260 CompilationPolicy::policy()->event(emh, mh,
>>>>> InvocationEntryBci, InvocationEntryBci, CompLevel_aot, cm, CHECK);
>>>>> 280 nmethod* osr_nm = CompilationPolicy::policy()->event(emh,
>>>>> mh, branch_bci, target_bci, CompLevel_aot, cm, CHECK);
>>>>>
>>>>> Nit: there's no need to use CHECK here.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>>
>>>>> 102 // Donot clear probable async exceptions.
>>>>>
>>>>> typo: s/Donot/Do not/
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/runtime/deoptimization.cpp
>>>>>
>>>>> 1686 void Deoptimization::load_class_by_index(const
>>>>> constantPoolHandle& constant_pool, int index) {
>>>>>
>>>>> This method should be declared with TRAPS now.
>>>>>
>>>>> 1693 // Donot clear probable Async Exceptions.
>>>>>
>>>>> typo: s/Donot/Do not/
>>>>>
>>>>>
>>>> Ok
>>>>>> testing : mach1-5(links in jbs)
>>>>>
>>>>> There is very little existing testing that will actually test the
>>>>> key changes you have made here. You will need to do direct
>>>>> fault-injection testing anywhere you now allow async exceptions to
>>>>> remain, to see if the calling code can tolerate that. It will be
>>>>> difficult to test thoroughly.
>>>>>
>>>> Ok
>>>>> Thanks again for tackling this difficult problem!
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> While working on JDK-8246381 it was noticed that compilation
>>>>>> request path clears all exceptions(including async) and doesn't
>>>>>> propagate[1].
>>>>>>
>>>>>> Fix: patch restores the propagation behavior for the probable
>>>>>> async exceptions.
>>>>>>
>>>>>> Compilation request path propagate exception as in [2]. MDO and
>>>>>> MethodCounter doesn't expect any exception other than metaspace
>>>>>> OOM(added comments).
>>>>>>
>>>>>> Deoptimization path doesn't clear probable async exceptions and
>>>>>> take unpack_exception path for non uncommontraps.
>>>>>>
>>>>>> Added java_lang_InternalError to well known classes.
>>>>>>
>>>>>> Request for review.
>>>>>>
>>>>>> Best Regards,
>>>>>>
>>>>>> Jamsheed
>>>>>>
>>>>>> [1] w.r.t changes done for JDK-7131259
>>>>>>
>>>>>> [2]
>>>>>>
>>>>>> (a)
>>>>>> ----->
>>>>>> c1_Runtime1.cpp/interpreterRuntime.cpp/compilerRuntime.cpp
>>>>>> |
>>>>>> ----- compilationPolicy.cpp/tieredThresholdPolicy.cpp
>>>>>> |
>>>>>> ------ compileBroker.cpp
>>>>>>
>>>>>> (b)
>>>>>> Xcomp versions
>>>>>> ------> compilationPolicy.cpp
>>>>>> |
>>>>>> ------> compileBroker.cpp
>>>>>>
>>>>>> (c)
>>>>>>
>>>>>> Direct call to compile_method in compileBroker.cpp
>>>>>>
>>>>>> JVMCI bootstrap, whitebox, replayCompile.
>>>>>>
>>>>>>
More information about the hotspot-runtime-dev
mailing list