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

[Daniel Fuchs]

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

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


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