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 <[email protected]> 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, [email protected] 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 <[email protected]>
>>>> 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@... 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
>>
>
