[foreign] RFR 8218669: Flatten Reference hierarchy

[Maurizio Cimadamore]

On 11/02/2019 11:33, Sundararajan Athijegannathan wrote:
> In LayoutTypeImpl.java, there is a new import for "Reference". Is it 
> used? Don't see any further changes in that file.
Yes, in a field:

     private final Reference reference;

Since Reference changed location (from toplevel to nested), imports have 
been tweaked.

Maurizio


> 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