[9] RFR (S): 8161720: Better byte behavior for off-heap data

Tue Aug 23 16:46:11 UTC 2016

Hi Volker,

thank you for the feedback!  Please see details below.

On 08/22/2016 06:30 PM, Volker Simonis wrote:
> Hi Zoltan,
>
> are you sure your tests are doing what they are supposed to do?
>
> If for example I run UnsafeOnHeapBooleanTest with '-Xcomp
> -XX:-TieredCompilation -XX:CompileCommand="option
> UnsafeOnHeapBooleanTest::main,PrintOptoAssembly"' I can see that
> UnsafeOnHeapBooleanTest::main will be compiled but it only contains an
> uncommon trap:
>
> 000   B1: #    N1 <- BLOCK HEAD IS JUNK   Freq: 1
> 000       # stack bang (216 bytes)
>      pushq   rbp    # Save rbp
>      subq    rsp, #16    # Create frame
>
> 00c       movl    RSI, #41    # int
>            nop     # 2 bytes pad for loops and calls
> 013       call,static  wrapper for: uncommon_trap(reason='unloaded'
> action='reinterpret' index='41' debug_id='0')
>          # UnsafeOnHeapBooleanTest::main @ bci:0  L[0]=_ L[1]=_ L[2]=_
> L[3]=_ L[4]=_ L[5]=_ L[6]=_ L[7]=_ L[8]=_
>          # OopMap{off=24}
> 018       int3    # ShouldNotReachHere
> 018
>
> So the test will be actually executed in the interpreter and not in
> the C2-compiled code as expected.
>
> I in generally prefer to put tests in a function and call them in a
> loop (with -XX:-UseOnStackReplacement) which I think is a much more
> stable way for exercising the different compilation levels.

That is a good idea!  I've changed the tests to work as you suggested.  
I also checked if the "interesting" code regions are compiled and that 
the compiled code is executed (it is).

>
> And I have a question about JNI: is it only a convention that JNI
> functions should return JNI_TRUE/JNI_FALSE or is this required by the
> specification? Unfortunately, bug "JDK-8149170: Better byte behavior
> for native arguments" which is mentioned in this bug description is
> not visible.

Here is an excerpt from the JNI Programmer's Guide & Specification [1]:

That piece of documentation was also mentioned during the work for 
"JDK-8149170: Better byte behavior for native arguments".

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8161720/webrev.01/

Local testing showed no problems, JPRT testing is in progress.

Thank you!

Best regards,

Zoltan

[1] 
https://web.archive.org/web/20120626012047/http://java.sun.com/docs/books/jni/html/pitfalls.html#30066

>
> Regards,
> Volker
>
> On Mon, Aug 22, 2016 at 4:25 PM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
>> Hi,
>>
>>
>> please review the fix for JDK-8161720.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8161720
>>
>>
>> Problem: As John pointed out in the bug description, when a boolean value is
>> loaded using Unsafe.getBoolean(), the VM should normalize the byte value
>> from the range 0..255 to 0..1.
>>
>> The patch I'm proposing includes a new test, UnsafeOffHeapBooleanTest.java
>> [1].  The test does the following (in essence):
>>
>> boolean bool1 = Unsafe.getBoolean(<off-heap memory region 1 containing a
>> non-null value, e.g., 0x2>);
>> boolean bool2 = Unsafe.getBoolean(<off-heap memory region 2 containing a
>> non-null value, e.g., 0x4>);
>> boolean result = bool1 & bool2; // Is supposed to be true
>>
>> The value of 'result' is supposed to be true.  However, because values
>> returned by Unsafe.getBoolean() are currently not normalized by the VM,
>> 'value' can be either true or false.  The value of 'result' depends on (1)
>> whether the code above is interpreted or compiled and (2) also on the
>> platform where the test is executed.
>>
>> The same problem can be triggered with on-heap accesses as well. Here is a
>> second test, UnsafeOnHeapBooleanTest.java [2] that does that.  I'd like to,
>> therefore, propose that the VM normalizes not only off-heap data read by
>> Unsafe.getBoolean() (as originally proposed by John), but also on-heap data
>> returned by Unsafe.getBoolean().
>>
>> Maybe the UnsafeOnHeapBooleanTest.java test mis-uses the Unsafe API in the
>> way it reads data from the heap.  But as any user of the Unsafe API could do
>> what the test does, maybe it would be good to "protect" the VM from the
>> problem of having to deal with many different 'true' values (e.g., 0x2 and
>> 0x4).  Also, Unsafe.getBoolean() is declared as a native, so maybe it's not
>> too bad if the method respects JNI conventions.
>>
>>
>> Solution: Normalize the result returned by Unsafe.getBoolean in
>> - src/share/vm/prims/unsafe.cpp (used by the interpreter and by compiled
>> code if the compiler intrinsics for Unsafe.getBoolean() are disabled)
>> - the C1 and C2 intrinsics for Unsafe.getBoolean().
>>
>>
>> Webrev:
>> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/
>>
>> Testing:
>> - JPRT (incl. Unsafe[On|Off]HeapBooleanTest.java);
>> - RBT testing with all hotspot tests both w/ -Xmixed and -Xcomp (no new
>> problems have showed up).
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>> [1]
>> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/test/compiler/unsafe/UnsafeOffHeapBooleanTest.java.html
>> [2]
>> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/test/compiler/unsafe/UnsafeOnHeapBooleanTest.java.html
>>