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

[Doug Simon]

On Thu, 14 Sep 2023 17:46:15 GMT, Tom Rodriguez <never at openjdk.org> wrote:

>> 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.

Instead, I generalized `getLiveObjectLocalsAt` to `getOopMapAt` since the VM computation is for both locals and operand stack anyway.
When called for OSR entry points, the result will be the same since (currently) HotSpot requires the stack to be empty.

>> 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.

Ok, I'm convinced.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1327321803
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1324933851