[foreign] RFR 8218669: Flatten Reference hierarchy

[Sundararajan Athijegannathan]

Okay,thanks.

-Sundar

On 11/02/19, 5:18 PM, Maurizio Cimadamore wrote:
>
>
> 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