RFR: 8208686: [AOT] JVMTI ResourceExhausted event repeated for same allocation
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Oct 5 19:56:12 UTC 2018
+1
Thanks,
Serguei
On 10/5/18 9:05 AM, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 10/5/18 8:22 AM, Doug Simon wrote:
>> Hi,
>>
>> Testing found a bug in the original webrev. Namely, when clearing out
>> a pending exception and returning null in the JVMCI ..._or_null
>> stubs, the JavaThread::_vm_result field was not being set to NULL.
>> I've addressed this in v2 of the webrev:
>>
>> Relative diff for bug fix:
>>
>> -----------------------------------------------------------------------------------------------
>>
>> -// Manages a scope in which a failed heap allocation will throw an
>> exception.
>> -// The pending exception is cleared when leaving the scope.
>> +// Manages a scope for a JVMCI runtime call that attempts a heap
>> allocation.
>> +// If there is a pending exception upon closing the scope and the
>> runtime
>> +// call is of the variety where allocation failure returns NULL
>> without an
>> +// exception, the following action is taken:
>> +// 1. The pending exception is cleared
>> +// 2. NULL is written to JavaThread::_vm_result
>> +// 3. Checks that an OutOfMemoryError is
>> Universe::out_of_memory_error_retry().
>> class RetryableAllocationMark: public StackObj {
>> private:
>> JavaThread* _thread;
>> public:
>> RetryableAllocationMark(JavaThread* thread, bool activate) {
>> if (activate) {
>> - assert(thread->in_retryable_allocation(), "retryable
>> allocation scope is non-reentrant");
>> + assert(!thread->in_retryable_allocation(), "retryable
>> allocation scope is non-reentrant");
>> _thread = thread;
>> _thread->set_in_retryable_allocation(true);
>> } else {
>> @@ -136,6 +141,7 @@
>> ResourceMark rm;
>> fatal("Unexpected exception in scope of retryable
>> allocation: " INTPTR_FORMAT " of type %s", p2i(ex),
>> ex->klass()->external_name());
>> }
>> + _thread->set_vm_result(NULL);
>> }
>> }
>> }
>> -----------------------------------------------------------------------------------------------
>>
>>
>> I also took the opportunity to factor out negative array length
>> checking:
>>
>> -----------------------------------------------------------------------------------------------
>>
>> diff -r 4d36f5998a8b src/hotspot/share/oops/arrayKlass.cpp
>> --- a/src/hotspot/share/oops/arrayKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/arrayKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -130,9 +130,6 @@
>> }
>> objArrayOop ArrayKlass::allocate_arrayArray(int n, int length,
>> TRAPS) {
>> - if (length < 0) {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_ARRAY), CHECK_0);
>> int size = objArrayOopDesc::object_size(length);
>> Klass* k = array_klass(n+dimension(), CHECK_0);
>> diff -r 4d36f5998a8b src/hotspot/share/oops/instanceKlass.cpp
>> --- a/src/hotspot/share/oops/instanceKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/instanceKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -1201,9 +1201,6 @@
>> }
>> objArrayOop InstanceKlass::allocate_objArray(int n, int length,
>> TRAPS) {
>> - if (length < 0) {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_NULL);
>> int size = objArrayOopDesc::object_size(length);
>> Klass* ak = array_klass(n, CHECK_NULL);
>> diff -r 4d36f5998a8b src/hotspot/share/oops/klass.cpp
>> --- a/src/hotspot/share/oops/klass.cpp Fri Oct 05 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/klass.cpp Fri Oct 05 17:17:17 2018 +0200
>> @@ -620,6 +620,8 @@
>> } else {
>> THROW_OOP(Universe::out_of_memory_error_retry());
>> }
>> + } else if (length < 0) {
>> + THROW_MSG(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> }
>> }
>> diff -r 4d36f5998a8b src/hotspot/share/oops/klass.hpp
>> --- a/src/hotspot/share/oops/klass.hpp Fri Oct 05 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/klass.hpp Fri Oct 05 17:17:17 2018 +0200
>> @@ -514,7 +514,7 @@
>> virtual Klass* array_klass_impl(bool or_null, int rank, TRAPS);
>> virtual Klass* array_klass_impl(bool or_null, TRAPS);
>> - // Error handling when length > max_length
>> + // Error handling when length > max_length or length < 0
>> static void check_array_allocation_length(int length, int
>> max_length, TRAPS);
>> void set_vtable_length(int len) { _vtable_len= len; }
>> diff -r 4d36f5998a8b src/hotspot/share/oops/objArrayKlass.cpp
>> --- a/src/hotspot/share/oops/objArrayKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/objArrayKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -170,14 +170,10 @@
>> }
>> objArrayOop ObjArrayKlass::allocate(int length, TRAPS) {
>> - if (length >= 0) {
>> - check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_0);
>> - int size = objArrayOopDesc::object_size(length);
>> - return (objArrayOop)Universe::heap()->array_allocate(this, size,
>> length,
>> - /* do_zero
>> */ true, THREAD);
>> - } else {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> + check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_0);
>> + int size = objArrayOopDesc::object_size(length);
>> + return (objArrayOop)Universe::heap()->array_allocate(this, size,
>> length,
>> + /* do_zero */
>> true, THREAD);
>> }
>> static int multi_alloc_counter = 0;
>> diff -r 4d36f5998a8b src/hotspot/share/oops/typeArrayKlass.cpp
>> --- a/src/hotspot/share/oops/typeArrayKlass.cpp Fri Oct 05
>> 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/typeArrayKlass.cpp Fri Oct 05
>> 17:17:17 2018 +0200
>> @@ -99,14 +99,10 @@
>> typeArrayOop TypeArrayKlass::allocate_common(int length, bool
>> do_zero, TRAPS) {
>> assert(log2_element_size() >= 0, "bad scale");
>> - if (length >= 0) {
>> - check_array_allocation_length(length, max_length(), CHECK_NULL);
>> - size_t size = typeArrayOopDesc::object_size(layout_helper(),
>> length);
>> - return (typeArrayOop)Universe::heap()->array_allocate(this,
>> (int)size, length,
>> - do_zero, CHECK_NULL);
>> - } else {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> + check_array_allocation_length(length, max_length(), CHECK_NULL);
>> + size_t size = typeArrayOopDesc::object_size(layout_helper(), length);
>> + return (typeArrayOop)Universe::heap()->array_allocate(this,
>> (int)size, length,
>> + do_zero, CHECK_NULL);
>> }
>> oop TypeArrayKlass::multi_allocate(int rank, jint* last_size,
>> TRAPS) {
>> -----------------------------------------------------------------------------------------------
>>
>>
>> Please confirm review these new changes:
>>
>> http://cr.openjdk.java.net/~dnsimon/8208686v2
>>
>> -Doug
>>
>>
>>> On 4 Oct 2018, at 00:20, Doug Simon <doug.simon at oracle.com> wrote:
>>>
>>> Thanks for the reviews Serguei and Vladimir.
>>>
>>> Unless I hear objections in the next 24 hours, I'll push this webrev.
>>>
>>> -Doug
>>>
>>>> On 3 Oct 2018, at 03:14, serguei.spitsyn at oracle.com wrote:
>>>>
>>>> Hi Doug,
>>>>
>>>> The JVMTI related part looks good to me.
>>>> Thank you for fixing it!
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 10/2/18 1:11 AM, Doug Simon wrote:
>>>>> It would be great to get some input from the non-compilers teams
>>>>> on this RFR.
>>>>>
>>>>> -Doug
>>>>>
>>>>>> On 28 Sep 2018, at 19:51, Vladimir Kozlov
>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>
>>>>>> To let you know, me and Tom R. did review these changes and
>>>>>> agreed that it is the least intrusive changes for Hotspot shared
>>>>>> code.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 9/25/18 8:11 AM, Daniel D. Daugherty wrote:
>>>>>>> Adding serviceability-dev at ... since this is JVM/TI...
>>>>>>> Dan
>>>>>>> On 9/25/18 10:48 AM, Doug Simon wrote:
>>>>>>>> A major design point of Graal is to treat allocations as
>>>>>>>> non-side effecting to give more freedom to the optimizer by
>>>>>>>> reducing the number of distinct FrameStates that need to be
>>>>>>>> managed. When failing an allocation, Graal will deoptimize to
>>>>>>>> the last side effecting instruction before the allocation. This
>>>>>>>> mean the VM code for heap allocation will potentially be
>>>>>>>> executed twice, once from Graal compiled code and then again in
>>>>>>>> the interpreter. While this is perfectly fine according to the
>>>>>>>> JVM specification, it can cause confusing behavior for JVMTI
>>>>>>>> based tools. They will receive 2 ResourceExhausted events for a
>>>>>>>> single allocation. Furthermore, the first ResourceExhausted
>>>>>>>> event (on the Graal allocation slow path) might denote a
>>>>>>>> bytecode instruction that performs no allocation, making it
>>>>>>>> hard to debug the memory failure.
>>>>>>>>
>>>>>>>> The proposed solution is to add an extra set of JVMCI VM
>>>>>>>> runtime calls for allocation. These entry points will attempt
>>>>>>>> the allocation and upon failure,
>>>>>>>> skip side-effects such as posting JVMTI events or handling
>>>>>>>> -XX:OnOutOfMemoryError. The compiled code using these entry
>>>>>>>> points is expected deoptmize on null.
>>>>>>>>
>>>>>>>> The path from these new entry points to where allocation can
>>>>>>>> fail goes through quite a bit of VM code. One could modify all
>>>>>>>> these paths by:
>>>>>>>> * Returning null instead of throwing an exception on failure.
>>>>>>>> * Adding a `bool null_on_fail` argument to all relevant methods.
>>>>>>>> * Adding extra null checking where necessary after each call to
>>>>>>>> these methods when `null_on_fail == true`.
>>>>>>>> This represents a significant number of changes.
>>>>>>>>
>>>>>>>> Instead, the proposed solution introduces a new
>>>>>>>> _in_retryable_allocation thread-local. This way, only the entry
>>>>>>>> points and allocation routines that raise an exception need to
>>>>>>>> be modified. Failure is communicated back to the new entry
>>>>>>>> points by throwing a special pre-allocated OOME object (i.e.,
>>>>>>>> Universe::out_of_memory_error_retry()) which must not propagate
>>>>>>>> back to Java code. Use of this object is not strictly
>>>>>>>> necessary; it is introduced to highlight/document the special
>>>>>>>> allocation mode.
>>>>>>>>
>>>>>>>> The proposed solution is at
>>>>>>>> http://cr.openjdk.java.net/~dnsimon/8208686.
>>>>>>>> THE JBS bug is: https://bugs.openjdk.java.net/browse/JDK-8208686
>>>>>>>>
>>>>>>>> -Doug
>>>>
>>>
>>
More information about the serviceability-dev
mailing list