vwithfield and ValueType.findWither

David Simms david.simms at oracle.com
Thu Jun 22 09:52:26 UTC 2017


Looks good to me, special thanks for adding more testing, excellent.

/David Simms


On 22/06/17 02:50, Paul Sandoz wrote:
> Here is webrev:
>
> http://cr.openjdk.java.net/~psandoz/valhalla/vwithfield/webrev/ 
> <http://cr.openjdk.java.net/%7Epsandoz/valhalla/vwithfield/webrev/>
>
> Including more updates to HS and a test.
>
> Paul.
>
>> On 20 Jun 2017, at 18:35, Paul Sandoz <paul.sandoz at oracle.com 
>> <mailto:paul.sandoz at oracle.com>> wrote:
>>
>> 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.javaMon 
>> Jun 19 11:07:52 2017 +0200
>> +++ 
>> b/src/java.base/share/classes/jdk/experimental/value/ValueType.javaTue 
>> 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.cppTue Jun 20 15:21:26 
>> 2017 +0200
>> +++ b/src/share/vm/interpreter/linkResolver.cppTue 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 
>>> <mailto: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 
>>>> <mailto: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/ 
>>>> <http://cr.openjdk.java.net/%7Edsimms/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