[aarch64-port-dev ] RFR: Bulk integration from Shenandoah

[Andrew Hughes]

On 16/03/2020 20:16, Roman Kennke wrote:
> Hi Andrew,
> 
>> On 06/03/2020 17:49, Aleksey Shipilev wrote:
>>> On 3/6/20 6:22 PM, Roman Kennke wrote:
>>>> Changeset:
>>>> http://cr.openjdk.java.net/~rkennke/bulk-integration-2020-03-06/changesets.01
>>>>
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~rkennke/bulk-integration-2020-03-06/webrev.01/
>>>
>>> This looks good to me.
>>>
>>> Andrews need to concur before we can push :)
>>>
>>
>> These would be easier to review if the cumulative effect of the changes
>> could be more easily seen. I was going to suggest including the merge
>> changeset, but this doesn't appear to be a merge.
> 
> Well yeah. The cumulative effect of the patches is exactly what you see
> here :-)

See where? The webrev has individual patches.

> 
>> Why are we not just
>> pushing these to aarch64/shenandoah-jdk8u as they are developed, rather
>> than waiting to do them all in bulk? Particularly as it's relatively
>> late in the development process for the April CPU.
> 
> I was thinking why are we integrating this to aarch64/jdk8u-shenandoah
> at all? We could also use shenandoah/jdk8 as integration forest.
> shenandoah/jdk8 has aarch64/jdk8 as upstream, which in turn has jdk8u as
> upstream. Dragging everything into aarch64/jdk8u-shenandoah is just a
> somewhat unnecessary extra step.

Funny, I was thinking you could just use aarch64/jdk8u-shenandoah. I
don't see why shenandoah/jdk8 is needed at all.

> 
>> Most of these seem to be Shenandoah-specific. The one that catches my
>> eye is:
>>
>> 8229919: Support JNI Critical functions in object pinning API on x86_32
>> platforms
>>
>> This seems like it will introduce a behavioural deviation for those who
>> are not using Shenandoah, in comparison to vanilla 8u. Can you assure me
>> this is safe?
> 
> Yes. This code is only entered when
> Universe::heap()->supports_object_pinning() returns true, and Shenandoah
> is currently the only implementation that does this. In the x86_32
> counterparts I actually put assert(UseShenandoahGC) in a couple of
> relevant places, I forgot this in x86_32. Sorry.

That sounds ok. It's certainly not obvious from the patchset though.

> 
>> I do wonder if patches like this should be backported to upstream 8u &
>> 11u, ahead of any proposed Shenandoah merge into those upstreams.
> 
> I was considering this. But it would be dead code upstream. So no.

Well, the point is it wouldn't be dead code if the long term plan was to
add Shenandoah. Getting shared code changes like this in first means the
final Shenandoah merge can largely be new code. It's the shared code
changes that affect other users.

> 
> The integration has already been pushed. I hope that's ok.
> x

Yes, I noticed when tagging the next build promotion.

> Roman
> 

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222