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

[Lindenmaier, Goetz]

Hi Andrew, 

Sorry I didn't mean to offend you.

But this thread is about optimizing the rules for 
changes Oracle downported to 11.  The majority of
these changes are not reviewed.  See the example
I assembled below.
So I just wanted to point to the existing rules, which 
say the maintainer must judge the risk. This is a 
mandatory step in the process.  A review is not
mandatory.

Obviously, any useful input is welcome to make 
the decision of the maintainer more easy, or to 
spot overseen risks.

The point of this discussion here is under which 
criteria Oracle changes should not be downported, see 
also Andrew H's last post:
http://mail.openjdk.java.net/pipermail/jdk-updates-dev/2020-September/003785.html

Best regards,
  Goetz.

I checked a snapshot of 19 changes I did for 11.0.9 (see below):
12 applied clean.
3 had a review because of Copyright/context conflicts.
4 had a review because I had to touch code.

clean https://bugs.openjdk.java.net/browse/JDK-8248472: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
code https://bugs.openjdk.java.net/browse/JDK-8248471: [aarch64] assert(false) failed: wrong size of mach node
code https://bugs.openjdk.java.net/browse/JDK-8248469: java.net HTTP/2 client does not decrease stream count when receives 204 response
clean https://bugs.openjdk.java.net/browse/JDK-8248229: 2020-04-24 public suffix list update v ff6fcea
code, relevant https://bugs.openjdk.java.net/browse/JDK-8248213: TestEliminateArrayCopy fails with -XX:+StressReflectiveCode
clean https://bugs.openjdk.java.net/browse/JDK-8248210: TestClone crashes with "all collected exceptions must come from the same place"
clean https://bugs.openjdk.java.net/browse/JDK-8248209: C1 assert(known_holder == NULL || (known_holder->is_instance_klass() && (!known_holder->is_interface() ...
copyright https://bugs.openjdk.java.net/browse/JDK-8248208: Possible NPE in ENC-PA-REP search in AS-REQ
clean https://bugs.openjdk.java.net/browse/JDK-8248207: Minimal fastdebug build broken after JDK-8245801
clean https://bugs.openjdk.java.net/browse/JDK-8248206: StressRecompilation triggers assert "redundunt OSR recompilation detected. memory leak in CodeCache!"
clean https://bugs.openjdk.java.net/browse/JDK-8248204: Enhance BaseLdapServer to support starttls extended request
clean https://bugs.openjdk.java.net/browse/JDK-8248203: [macos] macos10.14 Mojave returns anti-aliased glyphs instead of aliased B&W glyphs
context https://bugs.openjdk.java.net/browse/JDK-8248202: Jconsole can't connect to itself
clean https://bugs.openjdk.java.net/browse/JDK-8248201: java.rmi.NoSuchObjectException: no such object in table
clean https://bugs.openjdk.java.net/browse/JDK-8248197: Choose the default SecureRandom algo based on registration ordering
clean https://bugs.openjdk.java.net/browse/JDK-8248196: SSLSocket.getSession() doesn't close connection for timeout/ interrupts
context https://bugs.openjdk.java.net/browse/JDK-8248141: HttpConnection not returned to the pool after 204 response
code https://bugs.openjdk.java.net/browse/JDK-8246690: Tools should warn if weak algorithms are used before restricting them
clean https://bugs.openjdk.java.net/browse/JDK-8248002: Test javax/swing/border/TestTitledBorderLeak.java should be marked as headful



> -----Original Message-----
> From: Andrew Dinn <adinn at redhat.com>
> Sent: Thursday, September 10, 2020 11:46 AM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Andrew Haley
> <aph at redhat.com>; Doerr, Martin <martin.doerr at sap.com>; 'Severin
> Gehwolf' <sgehwolf at redhat.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>; jdk-
> updates-dev at openjdk.java.net
> Cc: Langer, Christoph <christoph.langer at sap.com>
> Subject: Re: [11u] Sconpe of Review ... was RFR(S): 8241234: Unify monitor
> enter/exit runtime entries.
> 
> On 09/09/2020 16:49, Lindenmaier, Goetz wrote:
> > It is pointless to ask reviewers to judge the risk
> > because reviews are only done if the change had to be adapted.
> > There are complex changes that just apply clean and thus are
> > downported without review.
> 
> I think you have very much overstated your case in that opening line. If
> a patch needs adapting in anything other than a trivial manner then it
> cannot be a bad thing for the reviewer to consider the risk involved.
> That benefit cannot be removed by the fact that some changes bypass the
> review process.
> 
> It might be diminished, of course. But by the same token, if  a complex
> patch does not need adapting then it might still be a good thing for the
> maintainer who is going to approve or disapprove it to consider the
> risk. Indeed, I hope that this consideration is foremost in the
> maintainers' minds.
> 
> Either way, it's far from /pointless/ for reviewers or maintainers to
> consider risks and where they think it important negotiate the details
> with the downporter. Likewise if the downporter is unsure then they
> really ought to initiate such a dialogue. Such actiuon might well be
> redundant in many cases but I am sure those will be easy to spot.
> 
> > Judging the risk is clearly a thing of the downporter. This is formulated
> > in Rule 1 of Oracles's Updates
> > description : http://openjdk.java.net/projects/jdk-updates/approval.html ,
> > and also mentioned in step 6 of jdk11us description:
> >
> https://wiki.openjdk.java.net/display/JDKUpdates/How+to+contribute+a+fix
> > This is supposed to help the maintainer to decide about the risk.
> 
> That would be fine if every downporter was happy and able to make the
> assessment. However, there are definitely going to be cases where that
> is not so.
> 
> I'm not sure why you are stating the requirements here in such absolute
> terms. Note that in my previous post I did not recommend that /all/
> responsibility be shifted to the reviewer or maintainer. I recommended
> that reviewers consider helping downporters with complex backports,
> including those that might benefit from trimming or re-implementing a fix.
> 
> > The major task of the review is to make sure the downported change
> > is correct, i.e. has the same effect on 11 as the original one.
> > Nevertheless, if there is a  reviewer and he feels bad about a
> > change, he should communicate his concerns!
> I think it is misguided to separate the decision as to correctness from
> concerns over the risk associated with a change. My experience is that
> cases where it is hard to foresee all the consequences of downporting a
> change are far from rare. For a lot of complex changes -- and even some
> simple ones -- a 100% foolproof assessment of correctness is never going
> to be available. In such cases the risk of breaking something really
> ought to be factored in by the reviewer because s/he can only speak to
> the /likely/ correctness (as opposed to /absolute/ correctness) of the
> patch.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Red Hat Distinguished Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill