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