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