RFR: 8315954: getArgumentValues002.java fails on Graal [v2]

[Tom Rodriguez]

On Thu, 14 Sep 2023 17:40:56 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 3099:
>> 
>>> 3097:   JVMCIPrimitiveArray oop_map = JVMCIENV->wrap(oop_map_handle);
>>> 3098:   int oop_map_len = JVMCIENV->get_length(oop_map);
>>> 3099:   if (nwords > oop_map_len) {
>> 
>> Should we sanity check against `mask.number_of_entries()`?  One wrinkle here is that `compute_one_oop_map` also computes information about the stack so the mask it computes can be larger than just max_locals.  For the purposes of OSR this doesn't matter as none of the JITs support OSR with a non-empty stack, so we would never call it for a bci with a non-empty stack.  So should we disallow calling it with a non-empty stack or just properly handle it by passing in an array long enough to contain `max_locals + max_stack`?
>
> We only look up the mask for locals and so ignore stack indexes in the mask altogether. I'm assuming therefore that `mask.is_oop(i)` can never hit any problems.
> Note that this API should be safe when called for *any* valid BCI, not just those for an OSR entry point. Even if called for a BCI with a non-empty stack, the current implementation simply ignores that part of the mask.

Yes that's implied by the name of the method. It would make me happy if there was a comment pointing out that we're explicitly ignoring whether the stack is non-empty and contains oops.

>> src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java line 788:
>> 
>>> 786: 
>>> 787:     @Override
>>> 788:     public long getLiveObjectLocalsAt(int bci, BitSet bigOopMap) {
>> 
>> This seems overly complicated to me.  Why isn't it simply:
>> 
>>    public BitSet getLiveObjectLocalsAt(int bci) {
>>         int locals = getMaxLocals();
>>         int nwords = ((locals - 1) / 64) + 1;
>>         long liveness[] = new long[nwords];
>>         compilerToVM().getLiveObjectLocalsAt(this, bci, liveness);
>>         return new BitSet(liveness);
>>     }
>
> In the common case we can avoid the overhead of allocating a long array and initializing each of it elements with a JNI upcall.

It's premature optimization.  This is called once per OSR compile and the call itself is doing a dataflow over the bytecodes which seems more expensive then a couple JNI calls.  The array can be filled in using `copy_bytes_to` from HotSpot, which is a pair of JNI calls.  You already have 2 JNI calls because of the unsafe allocation.  I can maybe accept that returning a long from the JVM to avoid an allocation for the < 64 case isn't a terrible idea but I can't see any universe where we care about that.

This method should be returning a BitSet not a long.  Having the caller do that fixup is super ugly.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1326324383
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1324912621