[foreign] RFR 8218669: Flatten Reference hierarchy

[Jorn Vernee]

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