[foreign] RFR 8218669: Flatten Reference hierarchy

[Jorn Vernee]

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