RFR: 8077846: improve locking strategy for readConfiguration(), reset(), and initializeGlobalHandlers()

[David Holmes]

On 27/04/2015 10:21 PM, David Holmes wrote:
> On 27/04/2015 5:28 PM, Daniel Fuchs wrote:
>> Hi David,
>>
>> On 4/27/15 3:21 AM, David Holmes wrote:
>>> Hi Daniel,
>>>
>>> On 25/04/2015 3:07 AM, Daniel Fuchs wrote:
>>>> Hi,
>>>>
>>>> Please find below a patch that tries to improve the locking
>>>> strategy in LogManager.
>>>>
>>>> The patch proposes to use a Reantrant lock to deal with
>>>> configurations changes in reset() and readConfiguration(),
>>>> and avoids lock contention in  initializeGlobalHandlers()
>>>>
>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8077846/webrev.00/
>>>
>>> How early in VM startup can logging be initialized and used? I just
>>> wonder whether the use of ReentrantLock changes that at all?
>>
>> The initialization will not happen before the first class calls
>> Logger.getLogger() or LogManager.getLogManager().
>> In particular, it does not happen when the LogManager.class is loaded
>> (that is: it no longer happens as  part of the class static
>> initialization).
>>
>> I see Alan replied to that question as well...
>
> I'm just wondering what anyone might be running with a customized core
> class that uses logging, and which may now fail due to the different
> requirements.

But then I just recalled that we used to use ReentrantLock via 
ConcurrentHashMap in Class, so introducing it early is not likely to 
cause any issue.

Cheers,
David

>>> Took me a while to understand the use of initializeGlobalHandlersDone.
>>> I think this comment:
>>>
>>> 1622         // If we are in the process of initializing global
>>> handlers we
>>> 1623         // also need to lock & wait (this case is indicated by
>>> 1624         // initializeGlobalHandlersDone == false).
>>>
>>> would be a little clearer as:
>>>
>>> 1622         // If we are in the process of initializing global
>>> handlers we
>>> 1623         // also need to acquire the lock to wait until it is
>>> complete
>>> 1624         // (this case is indicated by
>>> initializeGlobalHandlersDone == false).
>>
>> Exactly. Thanks!
>>
>>> But why are these initialized to true and not false ??
>>>  184     private volatile boolean initializeGlobalHandlersCalled = true;
>>>  185     private volatile boolean initializeGlobalHandlersDone = true;
>>
>> It doesn't make sense to try & create global handlers before the
>> configuration is read for the first time. In addition the first thing
>> that readConfiguration does is to call reset() which switches
>> initializeGlobalHandlersCalled to true to avoid that it gets initialized
>> before the Properties is later loaded.
>>
>> I agree that this initialization to 'true' is not intuitive.
>> My fingers had been itching several times to change it to false ;-)
>> but the 'true' value makes more sense once you look at the
>> initialization code path.
>>
>> I should probably add a comment:
>>
>> // These variables are initialized to true, to avoid trying
>> // to initialize global handlers before the configuration is read
>> // for the first time.
>
> Sounds good to me.
>
> Thanks,
> David
>
>>
>>>
>>> I think this code/comment:
>>>
>>> 1629             if (initializeGlobalHandlersCalled) return; //
>>> recursive call
>>>
>>>  would also be clearer as:
>>>
>>> 1629             if (initializeGlobalHandlersCalled)
>>>                      return; // recursive call or another thread did
>>> it already
>>
>> Thanks David!
>>
>> best regards,
>>
>> -- daniel
>>>
>>> Thanks,
>>> David
>>>
>>>> comments welcome,
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>