RFR(L) 8153224 Monitor deflation prolong safepoints (CR3/v2.03/6-for-jdk13)

Mon May 6 21:21:39 UTC 2019

Dan,

Looks good to me - this makes the timing dance something I can get my head around.

I do not see any timing holes here. I did not re-review the wiki - it said it was still WIP.

Couple of minor comments:
1. synchronizer.cpp comment lines 1119-1120
Could you possibly clarify in the comment that internal inflation happens at times
when it is not safe to stop for a safepoint?

2. synchronizer.cpp
1174 assertion: how could the ref_count be > 0 if this is on the free list? I get that we don’t
“reset” it, but shouldn’t the ref_count inc/dec match once you’ve handled the -max_jint?

3. How did you decide which tryLock() calls to put the check for DEFLATER_MARKER after?

Looking forward to the performance numbers.

I can’t resist any longer - link to our new superhero: DEFLATER MOUSE  - https://steamcommunity.com/sharedfiles/filedetails/?id=729760850

many thanks,
Karen

> On May 6, 2019, at 11:52 AM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
> 
> Greetings,
> 
> I had some discussions with Karen about a race that was in the
> ObjectMonitor::enter() code in CR2/v2.02/5-for-jdk13. This race was
> theoretical and I had no test failures due to it. The fix is pretty
> simple: remove the special case code for async deflation in the
> ObjectMonitor::enter() function and rely solely on the ref_count
> for ObjectMonitor::enter() protection.
> 
> During those discussions Karen also floated the idea of using the
> ref_count field instead of the contentions field for the Async
> Monitor Deflation protocol. I decided to go ahead and code up that
> change and I have run it through the usual stress and Mach5 testing
> with no issues. It's also known as v2.03 (for those for with the
> patches) and as webrev/6-for-jdk13 (for those with webrev URLs).
> Sorry for all the names...
> 
> Main bug URL:
> 
>     JDK-8153224 Monitor deflation prolong safepoints
>     https://bugs.openjdk.java.net/browse/JDK-8153224
> 
> The project is currently baselined on jdk-13+18.
> 
> Here's the full webrev URL:
> 
> http://cr.openjdk.java.net/~dcubed/8153224-webrev/6-for-jdk13.full/
> 
> Here's the incremental webrev URL:
> 
> http://cr.openjdk.java.net/~dcubed/8153224-webrev/6-for-jdk13.inc/
> 
> I have also updated the OpenJDK wiki to reflect the CR3 changes:
> 
> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
> 
> This version of the patch has been thru Mach5 tier[1-8] testing on
> Oracle's usual set of platforms. My Solaris-X64 stress kit run had
> no issues. Kitchensink8H on product, fastdebug, and slowdebug bits
> had no failures on Linux-X64; MacOSX fastdebug and slowdebug and
> Solaris-X64 release had the usual "Too large time diff" complaints.
> 12 hour Inflate2 runs on product, fastdebug and slowdebug bits on
> Linux-X64, MacOSX and Solaris-X64 had no failures. My Linux-X64
> stress kit is running right now.
> 
> I've done the SPECjbb2015 baseline and CR3 runs. I need to gather
> the results and analyze them.
> 
> Thanks, in advance, for any questions, comments or suggestions.
> 
> Dan
> 
> 
> On 4/25/19 12:38 PM, Daniel D. Daugherty wrote:
>> Greetings,
>> 
>> I have a small but important bug fix for the Async Monitor Deflation
>> project ready to go. It's also known as v2.02 (for those for with the
>> patches) and as webrev/5-for-jdk13 (for those with webrev URLs). Sorry
>> for all the names...
>> 
>> JDK-8222295 was pushed to jdk/jdk two days ago so that baseline patch
>> is out of our hair.
>> 
>> Main bug URL:
>> 
>>     JDK-8153224 Monitor deflation prolong safepoints
>>     https://bugs.openjdk.java.net/browse/JDK-8153224
>> 
>> The project is currently baselined on jdk-13+17.
>> 
>> Here's the full webrev URL:
>> 
>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/5-for-jdk13.full/
>> 
>> Here's the incremental webrev URL (JDK-8153224):
>> 
>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/5-for-jdk13.inc/
>> 
>> I still have to update the OpenJDK wiki to reflect the CR2 changes:
>> 
>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>> 
>> This version of the patch has been thru Mach5 tier[1-6] testing on
>> Oracle's usual set of platforms. Mach5 tier[7-8] is running now.
>> My stress kit is running on Solaris-X64 now. Kitchensink8H is running
>> now on product, fastdebug, and slowdebug bits on Linux-X64, MacOSX
>> and Solaris-X64. 12 hour Inflate2 runs are running now on product,
>> fastdebug and slowdebug bits on Linux-X64, MacOSX and Solaris-X64.
>> I'll start my my stress kit on Linux-X64 sometime on Sunday (after
>> my jdk-13+18 stress run is done).
>> 
>> I'll do SPECjbb2015 baseline and CR2 runs after all the stress
>> testing is done.
>> 
>> Thanks, in advance, for any questions, comments or suggestions.
>> 
>> Dan
>> 
>> 
>> On 4/19/19 11:58 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>> 
>>> I finally have CR1 for the Async Monitor Deflation project ready to
>>> go. It's also known as v2.01 (for those for with the patches) and as
>>> webrev/4-for-jdk13 (for those with webrev URLs). Sorry for all the
>>> names...
>>> 
>>> Main bug URL:
>>> 
>>>     JDK-8153224 Monitor deflation prolong safepoints
>>>     https://bugs.openjdk.java.net/browse/JDK-8153224
>>> 
>>> Baseline bug fixes URL:
>>> 
>>>     JDK-8222295 more baseline cleanups from Async Monitor Deflation project
>>>     https://bugs.openjdk.java.net/browse/JDK-8222295
>>> 
>>> The project is currently baselined on jdk-13+15.
>>> 
>>> Here's the webrev for the latest baseline changes (JDK-8222295):
>>> 
>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/4-for-jdk13.8222295
>>> 
>>> Here's the full webrev URL (JDK-8153224 only):
>>> 
>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/4-for-jdk13.full/
>>> 
>>> Here's the incremental webrev URL (JDK-8153224):
>>> 
>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/4-for-jdk13.inc/
>>> 
>>> So I'm looking for reviews for both JDK-8222295 and the latest version
>>> of JDK-8153224...
>>> 
>>> I still have to update the OpenJDK wiki to reflect the CR changes:
>>> 
>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>>> 
>>> This version of the patch has been thru Mach5 tier[1-3] testing on
>>> Oracle's usual set of platforms. Mach5 tier[4-6] is running now and
>>> Mach5 tier[78] will be run later today. My stress kit on Solaris-X64
>>> is running now. Linux-X64 stress testing will start on Sunday. I'm
>>> planning to do Kitchensink runs, SPECjbb2015 runs and my monitor
>>> inflation stress tests on Linux-X64, MacOSX and Solaris-X64.
>>> 
>>> Thanks, in advance, for any questions, comments or suggestions.
>>> 
>>> Dan
>>> 
>>> 
>>> On 3/24/19 9:57 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>> 
>>>> Welcome to the OpenJDK review thread for my port of Carsten's work on:
>>>> 
>>>>     JDK-8153224 Monitor deflation prolong safepoints
>>>>     https://bugs.openjdk.java.net/browse/JDK-8153224
>>>> 
>>>> Here's a link to the OpenJDK wiki that describes my port:
>>>> 
>>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>>>> 
>>>> Here's the webrev URL:
>>>> 
>>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/3-for-jdk13/
>>>> 
>>>> Here's a link to Carsten's original webrev:
>>>> 
>>>> http://cr.openjdk.java.net/~cvarming/monitor_deflate_conc/0/
>>>> 
>>>> Earlier versions of this patch have been through several rounds of
>>>> preliminary review. Many thanks to Carsten, Coleen, Robbin, and
>>>> Roman for their preliminary code review comments. A very special
>>>> thanks to Robbin and Roman for building and testing the patch in
>>>> their own environments (including specJBB2015).
>>>> 
>>>> This version of the patch has been thru Mach5 tier[1-8] testing on
>>>> Oracle's usual set of platforms. Earlier versions have been run
>>>> through my stress kit on my Linux-X64 and Solaris-X64 servers
>>>> (product, fastdebug, slowdebug).Earlier versions have run Kitchensink
>>>> for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product, fastdebug
>>>> and slowdebug). Earlier versions have run my monitor inflation stress
>>>> tests for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product,
>>>> fastdebug and slowdebug).
>>>> 
>>>> All of the testing done on earlier versions will be redone on the
>>>> latest version of the patch.
>>>> 
>>>> Thanks, in advance, for any questions, comments or suggestions.
>>>> 
>>>> Dan
>>>> 
>>>> P.S.
>>>> One subtest in gc/g1/humongousObjects/TestHumongousClassLoader.java
>>>> is currently failing in -Xcomp mode on Win* only. I've been trying
>>>> to characterize/analyze this failure for more than a week now. At
>>>> this point I'm convinced that Async Monitor Deflation is aggravating
>>>> an existing bug. However, I plan to have a better handle on that
>>>> failure before these bits are pushed to the jdk/jdk repo.
>>>> 
>>> 
>>> 
>> 
>> 
> 