RFR 8251118: BiasedLocking::preserve_marks should not have a HandleMark
Patricio Chilano
patricio.chilano.mateo at oracle.com
Tue Aug 11 20:37:05 UTC 2020
On 8/11/20 5:29 PM, Harold Seigel wrote:
> Hi Patricio,
>
> I think your modifications of TestFullGCALot.java are fine for testing
> your fix. No need for a brand new test.
Great, thanks for reviewing this Harold!
Patricio
> Thanks, Harold
>
> On 8/11/2020 4:11 PM, Patricio Chilano wrote:
>> Hi Harold,
>>
>> Thanks for looking at this.
>>
>> On 8/11/20 4:11 PM, Harold Seigel wrote:
>>> Hi Patricio,
>>>
>>> The fix looks good. Could you also add a test based on the failure
>>> discussed in the bug description?
>> Sure. From the mach5 job I run without the patch I see that
>> test/hotspot/jtreg/gc/TestFullGCALot.java fails pretty much
>> consistently on all platforms and tiers 1-3. What do you think about
>> adding a new run on that test with -XX:+UseBiasedLocking? Change
>> would be:
>>
>> diff --git a/test/hotspot/jtreg/gc/TestFullGCALot.java
>> b/test/hotspot/jtreg/gc/TestFullGCALot.java
>> --- a/test/hotspot/jtreg/gc/TestFullGCALot.java
>> +++ b/test/hotspot/jtreg/gc/TestFullGCALot.java
>> @@ -26,8 +26,9 @@
>> /*
>> * @test TestFullGCALot
>> - * @bug 4187687 8187819
>> + * @bug 4187687 8187819 8251118
>> * @summary Ensure no access violation when using FullGCALot
>> * @requires vm.debug
>> * @run main/othervm -XX:NewSize=10m -XX:+FullGCALot
>> -XX:FullGCALotInterval=120 gc.TestFullGCALot
>> + * @run main/othervm -XX:NewSize=10m -XX:+FullGCALot
>> -XX:FullGCALotInterval=120 -XX:+UseBiasedLocking gc.TestFullGCALot
>> */
>>
>> I would suggest adding a new run to FieldMonitor.java with
>> -XX:+UseBiasedLocking instead, but it doesn't fail as consistently.
>>
>> Alternatively a new test would be pretty much like
>> TestFullGCALot.java. The only change I would make is to explicitly
>> add a synchronized statement:
>>
>> /*
>> * @test TestFullGCOnSync
>> * @bug 8251118
>> * @run main/othervm -XX:NewSize=10m -XX:+FullGCALot
>> -XX:FullGCALotInterval=120 -XX:+UseBiasedLocking TestFullGCOnSync
>> */
>>
>> public class TestFullGCOnSync {
>> public static void main(String args[]) throws Exception {
>> Integer x = new Integer(10);
>> synchronized (x) {
>> System.gc();
>> System.out.println("Hello world!");
>> }
>> }
>> }
>>
>> Thanks,
>> Patricio
>>> Thanks, Harold
>>>
>>> On 8/11/2020 2:11 PM, Patricio Chilano wrote:
>>>> Hi all,
>>>>
>>>> Please review the following small fix which involves removing the
>>>> added HandleMark in BiasedLocking::preserve_marks():
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8251118
>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8251118/v1/webrev/
>>>>
>>>> I've inspected the callers of BiasedLocking::preserve_marks() and
>>>> they all have an assert that the current thread is the VMThread.
>>>> Since the VMThread creates a HandleMark object before executing a
>>>> VM operation the extra HandleMark added in 8249192 is not needed.
>>>> I've run tiers1-3 in mach5 with -XX:+UseBiasedLocking and without
>>>> the patch I get several crashes in BiasedLocking::restore_marks().
>>>> With the patch tests completed successfully.
>>>>
>>>> Thanks,
>>>> Patricio
>>>>
>>
More information about the hotspot-runtime-dev
mailing list