[11u] RFR(S): 8241234: Unify monitor enter/exit runtime entries.

[Doerr, Martin]

Hi,

seems like two different philosophies collide here.

1. Some people assume that all of Oracle's 11u changes should get integrated into the open version.
2. Others only want to take them on demand with a good reason.

I think there are good arguments for and against both ones.
Personally, I think approach 1. is better at the beginning of an updates branch while it may be reasonable to switch at some point of time.
At the moment, I still prefer to stay in sync with Oracle as far as we can.

Regarding this change, I don't see a high risk.
What it basically does is that it reuses better code which is already used by C2 for C1 and JVMCI compilers. So there's no substantial new code.
It's tested by GraalVM and by our internal testing. There are no known issues with it.

So I'd rather vote for taking it.

Best regards,
Martin


> -----Original Message-----
> From: Andrew Haley <aph at redhat.com>
> Sent: Freitag, 28. August 2020 16:36
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Severin Gehwolf'
> <sgehwolf at redhat.com>; Doerr, Martin <martin.doerr at sap.com>; 'hotspot-
> compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>; jdk-updates-dev at openjdk.java.net
> Subject: Re: [11u] RFR(S): 8241234: Unify monitor enter/exit runtime entries.
> 
> Hi,
> 
> On 28/08/2020 14:11, Lindenmaier, Goetz wrote:
> >
> >>> I'm not really happy with 11 staying behind 11-oracle in the JVMCI issue.
> >> What JVMCI issue is this? Please explain. All that I see is a faster
> >> "slow" locking path for monitors.
> >
> > This was meant as a more general comment. I wanted to address that
> > we don't integrate many of the JVMCI changes so the OpenJDK 11 is
> > probably not usable with graal.  The comment was not tailored to
> > this specific change.  Unfortunately our team has not the capacity
> > to look at JVMCI/graal.
> 
> Fair enough.
> 
> Now, let's think about the wider point.
> 
> Any change is bad because our users want, above all else,
> stability. So first we should avoid change.
> 
> In order to justify any change, I want backport patches to have a real
> justification. That is to say, they must have a real effect on a Java
> user's experience.  Fixing visible bugs obviously qualifies, as does a
> significant performance bump, as does meeting a new crypto
> specification, etc, etc.
> 
> The other good reason is improved stability, which includes better
> testing.
> 
> A real justification doesn't exclude "cleanups", as long as there is
> some other benefit, such as making making a proposed backport
> cleaner. But it has to be a backport that we are actually doing, not
> some unknown backport that might happen some day.
> 
> It may well be that the 8241234 fix has a definite performance
> advantage, in which case it might be a reasonable thing to do.
> The provided justifications were:
> 
> - Oracle has done so. There may be more backports in this area and I'd
>   expect less effort if we have the same code in the open version.
> - Performance is supposed to be better.
> - New code is much cleaner.
> 
> But even though the new code is much cleaner, it's a significant
> change in a very delicate area. Bugs in this are can take a long time
> to reveal themselves, usually under heavy load in a production
> situation.
> 
> I am not saying no to this patch. I am asking "Are you sure that this
> change is worth making the change?" Given that I doubt anyone will
> ever notice this change unless it breaks something important, I have
> my doubts.
> 
> So, anyone: is there any chance that this patch will break something?
> Is this change worth the churn?
> 
> --
> Andrew Haley  (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671