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

[David Holmes]

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.

>> 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
>