On 7/22/20 8:11 AM, coleen.phillim...@oracle.com wrote:
On 7/22/20 4:21 AM, Thomas Schatzl wrote:
Hi,
On 22.07.20 02:42, David Holmes wrote:
Hi Thomas,
I've looked at the incremental update and I am happy with that.
In the response to Serguei's review there were some comment updates
and new webrevs.
I also, prompted by you mentioning it, took a deeper look at the
biased-locking code to ensure it also keeps the MonitorInfo's
thread-confined, and to see whether the handshake versions could
themselves be susceptible to interference from safepoints (which
they can't as far as I can determine). And that all seems fine.
Thanks for looking at this again in more detail. I couldn't spot
problems either, but this is not code I am proficient with.
As per offline discussions I know that there has been an alternate
proposal for a completely localized fix in the stackwalker code that
simply retrieves the list of monitors, uses the length to create the
array, then re-retrieves the list of monitors to populate the array
(the length of which can't change as we are dealing with the current
thread). My only concern with that approach is the performance
impact if we have deep stacks with lots of monitors. There is a
microbenchmark for StackWalker in the repo:
open/test/micro/org/openjdk/bench/java/lang/StackWalkBench.java
but it doesn't test anything to do with monitor usage.
Ultimately I am good with either change, as long as it's being fixed.
I initially thought about this solution too, but had the same
concerns. Stacks can be really deep particularly with some frameworks
(maybe not fully materialized) but idk the actual impact of doing the
walk twice.
Potentially you could have different fixes for different versions.
Yes. These patches look good to me for JDK 15 and 16. We'll open an
RFE to consider the alternate patch after more performance testing for
future versions, but this fixes the problem and will not be difficult
to backport to JDK 11.
Coleen,
So it looks like you, David and Serguei are comfortable with the larger
patch for both JDK15 and JDK16. That's good news! It's also good news
that you think this not be difficult to backport to JDK11.
For a post RDP2 push to JDK15, the bug's "Fix in Release" value will
have to be changed to 15 and a request for approval made. I've attached
Mark R's email about the process. Follow the links and submit the
approval requests.
As for the alternate fix, I only came up with it as a lower risk alternative
that could be pushed this late in the JDK15 cycle and would be easy to push
to JDK11u-oracle. Right now it appears that we won't need it, but just in
case we get push back on JDK15 approval, I'll finish the due diligence
testing.
Thomas, thanks for tackling this issue and for your patience in the review
process. Also, thanks for the GC debugging patch!
Dan
Thank you!
Coleen
Thanks,
Thomas
--- Begin Message ---
Per the JDK 15 schedule [1], we are now in Rampdown Phase Two.
The overall feature set is frozen. No further JEPs will be targeted
to this release.
Per the JDK Release Process [2] we now turn our focus to P1 and P2
bugs, which can be fixed with approval [3]. Late enhancements are
still possible, with approval [4], but the bar is now extraordinarily
high.
If you’re responsible for any of the bugs on the RDP 2 candidate-bug
list [5] then please see JEP 3 for guidance on how to handle them.
- Mark
[1] https://openjdk.java.net/projects/jdk/15/#Schedule
[2] https://openjdk.java.net/jeps/3
[3] https://openjdk.java.net/jeps/3#Fix-Request-Process
[4] https://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process
[5] https://j.mp/jdk-rdp-2
--- End Message ---
