vwithfield and ValueType.findWither

Paul Sandoz paul.sandoz at oracle.com
Wed Jun 21 01:35:16 UTC 2017


Below is a patch for the first stage. Loosening the check in HS and updating findWither to use vwithfield.

A follow on patch will refine the access control checks in findWither and add more tests.

Paul.

jdk:
diff -r e3e5af317626 src/java.base/share/classes/jdk/experimental/value/ValueType.java
--- a/src/java.base/share/classes/jdk/experimental/value/ValueType.java	Mon Jun 19 11:07:52 2017 +0200
+++ b/src/java.base/share/classes/jdk/experimental/value/ValueType.java	Tue Jun 20 18:29:39 2017 -0700
@@ -274,33 +274,13 @@
         ValueHandleKey key = ValueHandleKind.WITHER.key(List.of(name, type));
         MethodHandle result = handleMap.get(key);
         if (result == null) {
-            MethodHandle mh = boxLookup.findGetter(boxClass(), name, type);
-            Field field = MethodHandles.reflectAs(Field.class, mh);
-            Class<?> erasedType = type.isPrimitive() ?
-                    type : Object.class;
-            Method unsafeMethod = Stream.of(UNSAFE.getClass().getDeclaredMethods())
-                    .filter(m -> m.getName().startsWith("put") &&
-                            Arrays.asList(m.getParameterTypes()).equals(Arrays.asList(Object.class, long.class, erasedType)))
-                    .findFirst().get();
-            long fieldOffset = UNSAFE.objectFieldOffset(field);
-            result = MethodHandleBuilder.loadCode(boxLookup, mhName("wither$" + name), MethodType.methodType(valueClass(), MethodHandle.class, valueClass(), type),
-                    C -> {
-                        C.withLocal("boxedVal", BytecodeDescriptor.unparse(boxClass()))
-                         .load(1)
-                         .vbox(boxClass())
-                         .store("boxedVal")
-                         .load(0)
-                         .load("boxedVal")
-                         .const_(fieldOffset)
-                         .load(2);
-                         MethodType unsafeMT = MethodType.methodType(unsafeMethod.getReturnType(), unsafeMethod.getParameterTypes());
-                         C.invokevirtual(MethodHandle.class, "invokeExact", BytecodeDescriptor.unparse(unsafeMT), false)
-                          .load("boxedVal")
-                          .vunbox(valueClass())
-                          .vreturn();
-                    }).bindTo(MethodHandles.lookup().unreflect(unsafeMethod).bindTo(UNSAFE));
+            String fieldType = BytecodeDescriptor.unparse(type);
+
+            result = MethodHandleBuilder.loadCode(valueLookup, mhName("wither$" + name), MethodType.methodType(valueClass(), valueClass(), type),
+                    C -> C.vload(0).load(1).vwithfield(valueClass(), name, fieldType).vreturn());
             handleMap.put(key, result);
         }
+        // @@@ special access check for read/write access
         //force access-check
         lookup.findGetter(boxClass(), name, type);
         return result;


hotspot:
diff -r c6878e14a2df src/share/vm/interpreter/linkResolver.cpp
--- a/src/share/vm/interpreter/linkResolver.cpp	Tue Jun 20 15:21:26 2017 +0200
+++ b/src/share/vm/interpreter/linkResolver.cpp	Tue Jun 20 18:31:01 2017 -0700
@@ -951,6 +951,22 @@
     stringStream ss;

     if (sel_klass != current_klass) {
+      // For Minimal Value Types check if the current class is an anonymous
+      // class whose host class is the Derived Value Type class (selected class)
+      // or the Value Capable Class (VCC)
+      if (byte == Bytecodes::_vwithfield) {
+        assert(sel_klass->is_value(), "Expected Value Type");
+        if (current_klass->is_instance_klass() && InstanceKlass::cast(current_klass)->is_anonymous()) {
+          Klass* host_class = InstanceKlass::cast(current_klass)->host_klass(); // Is host VCC of DVT ?
+
+          if (host_class == sel_klass  || // Is DVT
+              (host_class->is_instance_klass() && // Is VCC
+               InstanceKlass::cast(host_class)->get_vcc_klass() == sel_klass)) {
+            return;
+          }
+        }
+      }
+
       ss.print("Update to %s final field %s.%s attempted from a different class (%s) than the field's declaring class",
                 is_static ? "static" : "non-static", resolved_klass->external_name(), fd.name()->as_C_string(),
                 current_klass->external_name());

> On 19 Jun 2017, at 09:42, Paul Sandoz <Paul.Sandoz at oracle.com> wrote:
> 
> Hi David,
> 
> That is almost the same direction i was thinking about. Specifically, fix HS so that for an anon class accessibility check is deferred to the host class.
> 
> As you point out findWither now accepts a lookup:
> 
>  public MethodHandle findWither(Lookup lookup, String name, Class<?> type) throws NoSuchFieldException, IllegalAccessException {
>>    //force access-check
>    lookup.findGetter(boxClass(), name, type);
>    return result;
> }
> 
> I am proposing that for findWither the developer pass a lookup created from MHs.privateLookupIn, thereby allowing to bypass the explicit access control checks in a controlled manner. IMO we don’t have to relax the access constraints on vwithfield.
> 
> Separately we can improve the user experience by focusing on MHs for constructors.
> 
> Paul.
> 
>> On 19 Jun 2017, at 02:33, David Simms <david.simms at oracle.com> wrote:
>> 
>> 
>> Re: "HotSpot code accessibility checks"
>> 
>> I did play around with explicitly opening up access from the VCC:
>> 
>> http://cr.openjdk.java.net/~dsimms/valhalla/vwithfield_access/webrev1/
>> 
>> (MethodHandleBuilder "vdefault" and "vwithfield" parts are already in)
>> 
>> This code is specific to the VCC / DVT relationship, and was a bit of a hack to start the conversation some time back. We were happy living with the unsafe hack for the time being, but I would prefer to use stop using said work-around.
>> 
>> I believe Dan's suggestion "Access restriction for vwithfield" on the design mail list is more practical (package private)...let's follow that conversation and get the JVMS definition right.
>> 
>> Main points from previous discussions:
>> 
>> * Original "vnew" proposal took all fields as stack args, all the time.
>>    o Hard to optimize, inefficient for value where most fields are
>>      "null/zero".
>>    o "vwithfield" easier to optimize (possible to elide COW
>>      semantics, efficiently change only required fields)
>> * Allow the user-code to enforce invariant values
>>    o Able to implement "never publish (vastore, putfield) 'Foo' value
>>      with any zero fields"
>>    o Access restrictions from VCC or package private ?
>> 
>> Recent changes to the ValueType API accepting MH lookup now gives a few more options to the user.
>> 
>> /D
>> 
>> On 15/06/17 19:32, Paul Sandoz wrote:
>>> Hi Fred,
>>> 
>>> Do you know where in the HotSpot code accessibility checks are performed for vwithfield? We need to fix those checks for Unsafe defined anonymous classes to defer to the host class (and if a suitable VCC or DVT then access should be granted).
>>> 
>>> Paul.
>>> 
>>>> On 15 Jun 2017, at 07:06, Frederic Parain <frederic.parain at oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 06/15/2017 05:36 AM, Maurizio Cimadamore wrote:
>>>>> 
>>>>> On 15/06/17 09:46, John Rose wrote:
>>>>>> On Jun 14, 2017, at 2:20 PM, Maurizio Cimadamore
>>>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>>>> Another possible story is that we always create values through the
>>>>>>> VCC constructor, and then unbox. If we think we're happy with that,
>>>>>>> that might be a fine choice too.
>>>>>> That's not a bad story for the JIT with aggressive scalarization,
>>>>>> and was my first idea about this, but it is painful in the interpreter.
>>>>>> 
>>>>> In which ways it's bad for the interpreter? Note that the current
>>>>> implementation of the 'modified' findWither will do just that:
>>>>> 
>>>>> * box incoming value
>>>>> * set field (with Unsafe.putXYZ)
>>>>> * unbox resulting reference back to a value
>>>>> 
>>>>> So, it seems to me that, to set 5 fields, the above idiom will do
>>>>> box/unbox 5 times, while if we just create a VCC with the fields we want
>>>>> and then unbox, that's just one unboxing step.
>>>>> 
>>>>> What am I missing?
>>>> With the vwithfield bytecode, the interpreter can avoid Java heap
>>>> allocations, with the box/Unsafe/unbox it cannot.
>>>> 
>>>> Just a performance/memory issue.
>>>> 
>>>> Fred
>> 
>> 
> 



More information about the valhalla-dev mailing list