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

[Zoltán Majó]

Hi John,


On 08/24/2016 03:33 AM, John Rose wrote:
> On Aug 22, 2016, at 7:25 AM, 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.
> You are right about this; good catch!
>
> Panama is likely to use primitive array types like "long[]" for holding arbitrary random
> native data structures, so we will see more "mismatched" accesses on the JVM heap
> in the future.  Since T_BOOLEAN is not a native type, those won't be likely, but let's
> be safe about it; if there is a possible mismatch (as your test creates), then we need
> to normalize the loaded boolean.
>
> The "on heap" optimization I was going for is for simple, type-safe reflective uses of U.getBool.
> So, (a) if the effective address of the getBool is on-heap, and (b) it is really typed as boolean
> (a field or array element), then the "!=0" stuff should be removed.  Otherwise, the unsafe
> access will be slower than the safe one, even after optimization, which is not what we are aiming at.

thank you for the clarification!

> How about this:   In library_call.cpp, you add some normalization code, under the condition
> "if_then(p, BoolTest::ne, ideal.ConI(0))".  Let's make that whole new block of code be gated
> under certain conditions, like this:
>
>      if (mismatched || heap_base_oop == top() || TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop))) {
>        // mismatched or (possibly) off-heap access
>        IdealKit ideal = IdealKit(this);
>        ...
>      }

I've implemented the check you suggested above.  I also added the 
optimization that we've discussed about in private.  In the updated 
webrev, the condition looks the following way:

         if (mismatched ||
             heap_base_oop == top() ||
(TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop)) && 
alias_type->field() == NULL))
         {
             // emit != 0 transformation
         }

As you mentioned, the last condition means that if (1) heap_base_oop is 
potentially NULL (and thus potentially off-heap) and (2) the offset of 
the unsafe access is small (at most the size of largest field offset 
possible), we don't emit the normalization.

The virtual address (0x0 + a small offset) is most usually not mapped.  
Accesses to that virtual address will most likely result in an SIGSEGV 
(or similar error). Therefore, we don't risk anything not normalizing 
values coming from that address (as we fail before returning the value).

The newly added test, UnsafeSmallOffsetBooleanAccessTest.java, is an 
example for skipping normalization potential off-heap accesses to small 
offsets.  That is, when the method test() is compiled,
- TypePtr::NULL_PTR->higher_equal(_gvn.type(heap_base_oop)) returns true 
(because we potentially have an instance of T as a first parameter to 
the getBoolean() call
- alias_type->field() is not NULL, because we access the same offset, no 
matter what the first parameter is.

The test passes because it does not actually access that small address.  
But it is a way to illustrate the optimization you've suggested.

Here is the updated webrev (relative to Vladimir Ivanov's latest 8155635 
and 8162101 changes):
- hotspot: http://cr.openjdk.java.net/~zmajo/8161720/webrev.02/hotspot/
- jdk: http://cr.openjdk.java.net/~zmajo/8161720/webrev.02/jdk/

(I forgot to include the jdk changeset into the previous webrev -- only 
one line was changed in the jdk).

Testing with JPRT passes.  I'll run RBT as well unless there are 
suggestions for other improvements.

Thank you!

Best regards,


Zoltan

>
> Thanks for attacking this one.
>
> — John
>
>> 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
>>