vwithfield and ValueType.findWither

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Jun 22 12:13:03 UTC 2017


+1

Maurizio


On 22/06/17 10:52, David Simms wrote:
>
> 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