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.si...@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.spit...@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.koz...@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@... 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
