JNI was <was> Re: RFR: 8183546: [MVT] cleanup/improve vreturn convention implementation

[Paul Sandoz]

Slight change in behaviour with the patch applied. Now i observe null returned if ValueTypeReturnedAsFields is enabled or disabled. (Note this behaviour is observed when running with -Xint).

This is currently what i am using:

  http://cr.openjdk.java.net/~psandoz/valhalla/unsafe-value-jdk/webrev/ <http://cr.openjdk.java.net/~psandoz/valhalla/unsafe-value-jdk/webrev/>
  http://cr.openjdk.java.net/~psandoz/valhalla/unsafe-value-hotspot/webrev/ <http://cr.openjdk.java.net/~psandoz/valhalla/unsafe-value-hotspot/webrev/>

Paul.

> On 6 Jul 2017, at 06:46, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 
> Sigh, it would help if I apply the patch! Sorry for the noise I will report back after testing with the applied patch.
> 
> Paul.
> 
>> On Jul 5, 2017, at 18:17, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>> 
>> Unfortunately i still observe the same errors with ValueTypeReturnedAsFields being enabled or disabled for the identity JNI function e.g.:
>> 
>> UNSAFE_ENTRY(jobject, Unsafe_GetValueIdentity(JNIEnv *env, jobject unsafe, jobject jvalue)) {
>> oop value = JNIHandles::resolve(jvalue);
>> assert(value->klass()->is_value(), "jvalue must be an value");
>> 
>> return jvalue;
>> } UNSAFE_END
>> 
>> When ValueTypeReturnedAsFields is enabled the error is:
>> 
>> V  [libjvm.dylib+0x102dcd4]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x5a4
>> V  [libjvm.dylib+0x102e578]  VMError::report_and_die(Thread*, char const*, int, char const*, char const*, __va_list_tag*)+0x78
>> V  [libjvm.dylib+0x699b9f]  report_vm_error(char const*, int, char const*, char const*, ...)+0x19f
>> V  [libjvm.dylib+0xd4d63e]  Monitor::check_prelock_state(Thread*)+0x9e
>> V  [libjvm.dylib+0xd4d28c]  Monitor::lock(Thread*)+0x9c
>> V  [libjvm.dylib+0x4f07b4]  MutexLocker::MutexLocker(Monitor*, Thread*)+0xb4
>> V  [libjvm.dylib+0x4eff15]  MutexLocker::MutexLocker(Monitor*, Thread*)+0x25
>> V  [libjvm.dylib+0xf65082]  SystemDictionary::add_loader_constraint(Symbol*, Handle, Handle, Thread*)+0x192
>> V  [libjvm.dylib+0xf6540b]  SystemDictionary::check_signature_loaders(Symbol*, Handle, Handle, bool, Thread*)+0xfb
>> V  [libjvm.dylib+0xb9cb7d]  LinkResolver::check_method_loader_constraints(LinkInfo const&, methodHandle const&, char const*, Thread*)+0xcd
>> V  [libjvm.dylib+0xb9c0e9]  LinkResolver::resolve_method(LinkInfo const&, Bytecodes::Code, Thread*)+0x649
>> V  [libjvm.dylib+0xb9ba05]  LinkResolver::resolve_method_statically(Bytecodes::Code, constantPoolHandle const&, int, Thread*)+0x2d5
>> V  [libjvm.dylib+0x37497d]  Bytecode_invoke::static_target(Thread*)+0x7d
>> V  [libjvm.dylib+0xeb3a28]  SharedRuntime::store_value_type_fields_to_buf(JavaThread*, long)+0x218
>> v  ~RuntimeStub::store_value_type_fields_to_buf
>> j  runtime.valhalla.valuetypes.UnsafeTest.main([Ljava/lang/String;)V+30
>> 
>> 
>> When ValueTypeReturnedAsFields is disabled a null value is returned. I tried detecting whether the value oop is in the heap with:
>> 
>> Universe::heap()->is_in_reserved(value)
>> 
>> and reallocating but it made no difference. I also started debugging HS but i don’t understand enough about how the template interpreter works to “interpret” the behaviour and track operations on the returned object.
>> 
>> There is something asymmetric about the handling of jobject value arguments and jobject value return.
>> 
>> 
>> Further more, does the creation of a jobject from a value oop require that the value be allocated on the heap? I think so otherwise i get failures for assertions such as:
>> 
>> assert(Universe::heap()->is_in_reserved(obj), "sanity check");
>> 
>> Paul.
>> 
>> 
>>> On 5 Jul 2017, at 10:51, Paul Sandoz <Paul.Sandoz at oracle.com> wrote:
>>> 
>>> 
>>>> On 5 Jul 2017, at 00:33, Roland Westrelin <rwestrel at redhat.com> wrote:
>>>> 
>>>> 
>>>> http://cr.openjdk.java.net/~roland/valhalla/vreturn-improv/webrev.00/
>>>> 
>>>> This patch:
>>>> 
>>>> - reworks the verification logic in
>>>> SharedRuntime::store_value_type_fields_to_buf() to address the crash
>>>> that Paul encountered
>>>> 
>>> 
>>> Thanks. I will verify this week when i update the Unsafe methods to be in sync with Fred’s latest changes.
>>> 
>>> Paul.
>>> 
>>>> - changes the vreturn convention so if we return the klass pointer +
>>>> fields, the klass pointer is tagged by setting its least significant
>>>> bit. This way it's cheaper for the caller to detect if it's getting an
>>>> oop, a buffered value or a list of fields back from the callee. (this
>>>> is based on a discussion with Fred)
>>>> 
>>>> - stores precomputed extended signature and vreturn registers in the
>>>> ValueKlass to not have to compute them every time they are
>>>> needed. This is also something Fred suggested. I considered
>>>> initializaing those fields on first use but came to the conclusion it
>>>> would make things too complicated. For instance the vreturn registers
>>>> are sometimes use at a point where the thread can't safepoint making
>>>> it impossible to use a metadata Array.
>>>> 
>>>> Roland.
>>> 
>> 
>