On Fri, 24 Sep 2021 17:53:03 GMT, Naoto Sato <na...@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
