[foreign] RFR 8218669: Flatten Reference hierarchy

[Sundararajan Athijegannathan]

In LayoutTypeImpl.java, there is a new import for "Reference". Is it 
used? Don't see any further changes in that file.

Other than that, looks good.

-Sundar


On 09/02/19, 1:28 AM, Jorn Vernee wrote:
> Tests pass on my machine as well.
>
> Cheers,
> Jorn
>
> Jorn Vernee schreef op 2019-02-08 19:09:
>> Sorry, this has gotten out of scope of the review.
>>
>> I think the code looks good, will run the tests as well.
>>
>> Jorn
>>
>> Jorn Vernee schreef op 2019-02-08 19:04:
>>> Maurizio Cimadamore schreef op 2019-02-08 18:30:
>>>> On 08/02/2019 16:41, Jorn Vernee wrote:
>>>>> Hi Maurizio,
>>>>>
>>>>> The OfDouble reference no longer checks the size of the type to 
>>>>> determine how to get/set the value. It looks like that case was 
>>>>> already handled by LayoutType.ofDouble by using 
>>>>> References.OfLongDouble?
>>>>
>>>> Yeah, not sure how that one got there. LayoutType::ofDouble creates
>>>> always the right reference (either OfDouble or OfLongDouble).
>>>>
>>>> Do you agree that the code as written now should be good?
>>>
>>> Yes, what you have now looks good to me as well. (I was curious)
>>>
>>>>>
>>>>> Should AbstractReference and Reference be merged as well? (and 
>>>>> then use pass up 'null' in OfGrumpy, since getter()/setter() 
>>>>> throws any ways)
>>>>
>>>> Not sure I understand what you mean - but this seems 1-1 with that 
>>>> we had?
>>>>
>>>> I don't think we want to stash the MHs in 'instance' fields, and then
>>>> have getter()/setter() return them - as that would be more opaque.
>>>
>>> Sorry, I was looking at the wrong side of the diff for a second.
>>> (using sdiff, so sometimes one side of the diff gets pushed off
>>> screen).
>>>
>>>>>
>>>>> What about profile pollution in the case of struct & array types? 
>>>>> It looks like at least OfStruct could benefit from some 
>>>>> specialization per carrier type.
>>>>
>>>> I think the tension there is that if you specialize by carrier, then
>>>> the MHs become less static (e.g. they end up depending on some
>>>> instance field of the reference), so not-so-constant from a VM
>>>> perspective (but perhaps @Stable could help).
>>>
>>> I'm mostly looking at the reflective constructor call in
>>> OfStruct::get, as well as the fact that it's looking up the struct
>>> impl class. Afaik those lookups would only have to be done once. So I
>>> would construct the MethodHandle something like this:
>>>
>>>         public static MethodHandle specializedGetter(Class<?> 
>>> carrier) {
>>>             Class<?> impl = 
>>> LibrariesHelper.getStructImplClass(carrier);
>>>             MethodHandles.Lookup lookup = MethodHandles.lookup();
>>>             try {
>>>                 MethodHandle cons =
>>> lookup.unreflectConstructor(impl.getConstructor(Pointer.class));
>>>                 MethodHandle check =
>>> lookup.findVirtual(BoundedPointer.class, "checkAlive",
>>> MethodType.methodType(void.class));
>>>                 check = check.asType(MethodType.methodType(void.class,
>>> Pointer.class));
>>>                 cons = MethodHandles.collectArguments(cons, 0, check);
>>>                 cons = MethodHandles.permuteArguments(cons,
>>> MethodType.methodType(impl, Pointer.class), 0, 0);
>>>                 return cons;
>>>             } catch (ReflectiveOperationException ex) {
>>>                 throw new IllegalStateException(ex);
>>>             }
>>>         }
>>>
>>> So there is no reference to any fields, the values are just fixed into
>>> the returned MethodHandle as constants (right?). And then create such
>>> a method handle per carrier. (and maybe mark
>>> LayoutTypeImpl::refecrence field @Stable as well?)
>>>
>>>> With arrays we could specialize by size. But for both I'm not 100%
>>>> clear of what the payoff would be. I'd prefer to keep these
>>>> refactorings limited to things that have come up in JMH runs. Profile
>>>> pollution with primitives was defo a problem (addressed by this
>>>> patch); it's likely there will be others, but proactively fix issues
>>>> w/o perf evidence to back things up could backfire.
>>>
>>> Yes I agree with this. It would be nice to have a standard set of
>>> benchmarks in the repo to use for that. jdk/jdk recently had a
>>> benchmark suite added [1], could we take advantage of that
>>> infrastructure?
>>>
>>> Thanks,
>>> Jorn
>>>
>>> [1] : https://bugs.openjdk.java.net/browse/JDK-8050952
>>>
>>>> Maurizio
>>>>
>>>>
>>>>>
>>>>> Jorn
>>>>>
>>>>> Maurizio Cimadamore schreef op 2019-02-08 13:12:
>>>>>> Hi,
>>>>>> this is a simple change which makes the Reference hierarchy in
>>>>>> References.java flatter and 1-1 with respect to the carrier it 
>>>>>> models.
>>>>>>
>>>>>> Some internal testing of linkToNative pointed at profile pollution
>>>>>> because of multiple carriers using the same reference class (e.g.
>>>>>> OfPrimitive).
>>>>>>
>>>>>> This patch does 3 things:
>>>>>>
>>>>>> 1) Remove the intermediate AbstractReference class
>>>>>> 2) Create one reference class per carrier, and make all 
>>>>>> getter/setter
>>>>>> MH static constants
>>>>>> 3) Consolidate the code my moving the Reference interface inside 
>>>>>> References.
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8218669/
>>>>>>
>>>>>> Cheers
>>>>>> Maurizio