[foreign] RFR 8218669: Flatten Reference hierarchy
Jorn Vernee
jbvernee at xs4all.nl
Fri Feb 8 18:04:47 UTC 2019
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
More information about the panama-dev
mailing list