[foreign] RFR 8218669: Flatten Reference hierarchy

[Jorn Vernee]

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