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

Mon May 6 22:35:59 UTC 2019

Karen,

Thanks for the review.

 > I did not re-review the wiki - it said it was still WIP.

I forgot to update the wiki to remove the WIP markers when I uploaded
the webrev this AM... The wiki is ready to be reviewed.

I'll reply to the other comments tomorrow.

Dan

On 5/6/19 5:21 PM, Karen Kinnear wrote:
> 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.
>>>>>
>>>>
>>>