<i18n dev> RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]

[Jaikiran Pai]

On Fri, 24 Sep 2021 17:53:03 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Minor test adjustments as suggested by Naoto
>>  - use a holder instead of volatile, as suggested by Roger
>
> src/java.base/share/classes/sun/util/calendar/CalendarSystem.java line 123:
> 
>> 121:      */
>> 122:     public static Gregorian getGregorianCalendar() {
>> 123:         var gCal = GREGORIAN_INSTANCE;
> 
> Do we need the local variable `gCal`?

This was there to avoid additional volatile reads in that method. A performance optimization. However, with the change Roger suggested, this is no longer relevant.

> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 43:
> 
>> 41:  * @run main/othervm CalendarSystemDeadLockTest
>> 42:  * @run main/othervm CalendarSystemDeadLockTest
>> 43:  * @run main/othervm CalendarSystemDeadLockTest
> 
> Just curious, before the fix, how many test instances caused the deadlock? I'd think these 5 runs are arbitrary numbers, Just wanted to have those 5 runs are appropriate.

Hello @naotoj, 
On my setup, without the fix, the test deadlocks almost always right on the first run. There have been cases where it did pass the first time, but running it a second time has always reproduced the failure. The 5 runs that I have in this test is indeed an arbitrary number. Given how quickly this test completes, I decided to use a slightly higher number of 5 instead of maybe 2 or 3. Do you think, we should change the run count to something else?

> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 75:
> 
>> 73:         tasks.add(new GetGregorianCalTask(taskTriggerLatch));
>> 74:         tasks.add(new GetGregorianCalTask(taskTriggerLatch));
>> 75:         final ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
> 
> Asserting `tasks.size() == numTasks` may help here.

Yes, that makes sense. I've updated the test to add this check.

> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 171:
> 
>> 169:         }
>> 170:     }
>> 171: }
> 
> Need a new line at the EOF.

Done. I've updated this in the latest version of the PR.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5683