RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship

[Daniel Fuchs]

Hi Martin,

Thanks for the cheers!  :-)

I am incorporating your comments.
Thanks for spotting these issues!

best regards,

-- daniel

On 9/5/13 6:49 PM, Martin Buchholz wrote:
> Random review comments:
>
> Let me cheerlead your efforts to clean up j.u.logging.  It's not easy.
>   Previous efforts (including by myself) have been incomplete because of
> failure to fully understand the big picture.
>
> I've thought that the right approach for write-mostly data structure
> like in j.u.logging (and in e.g. classloading) is to have
> mostly-immutable parts with volatile pointers that are updated using CAS.
>
> You have some typos:
> LogMnager
> initalization
> initializationCalled
>
> +     *
> +     **/
>
> =>
>
>        */
>
>
>
> On Thu, Sep 5, 2013 at 7:43 AM, Daniel Fuchs <daniel.fuchs at oracle.com
> <mailto:daniel.fuchs at oracle.com>> wrote:
>
>     Hi,
>
>     Here is the new patch. It uses an additional volatile flag
>     to avoid synchronizing once the log manager is fully initialized.
>     I have added some comments to spell out the purpose of the flags,
>     as well as some assertion that should make it clear what is expected,
>     what is possible, and what is not.
>
>     I have also implemented the other remarks from David, Peter, and Mandy.
>
>     <http://cr.openjdk.java.net/~__dfuchs/webrev_8023168/webrev.__01/
>     <http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.01/>>
>
>     Testing is still under way - but the results so far are looking good.
>     I also ran the JCK for api/java_util - nothing jumped at my face.
>
>     I have modified my NetBeans configuration to run NetBeans 7.3 over
>     my private build - and I'm not seeing any troubles either so far.
>
>     (I had done that with the previous patch too)
>
>     best regards,
>
>     -- daniel
>
>
>
>     On 9/5/13 2:43 PM, Daniel Fuchs wrote:
>
>         On 9/5/13 1:51 PM, Peter Levart wrote:
>
>                 Threads that call ensureLogManagerInitialized() after
>                 that will find
>                 the flag
>                 is true before entering the synchronized block and will
>                 return directly.
>
>
>             ...and such threads can see rootLogger field being either
>             still null or
>             not null but the Logger instance that is obtained via such
>             data race can
>             be half-initialized.
>
>
>         I don't think so, because the flag isn't set to true until
>         the initialization is finished.
>
>         BTW - there was a reason for having both an initializationCalled
>         *and* a synchronized at method level. The purpose of
>         initializationCalled flag was to prevent infinite recursion in the
>         same thread - because addLogger within ensureLogManagerInitialized
>         will trigger a new call to ensureLogManagerInitialized that the
>         synchronized won't block - since it's in the same thread.
>
>         So I need in fact two flags: one initializationDone - which will
>         be volatile - the other initializationCalled - which doesn't need
>         to be since it will be always written/read from within the
>         synchronized block.
>
>         I toyed with the idea of using a single volatile three valued byte
>         instead:
>
>         initialization: 0 => uninitialized, 1 => initializing, 2 =>
>         initialized
>
>         I opted for the two booleans - but the three valued byte might
>         be more
>         efficient:
>
>         1)
>
>         volatile byte initialization = 0;
>
>         ensureLogManagerInitialized() {
>             if (initialization == 2) return; // already done
>             synchronized(this) {
>               if (initialization > 0) return;
>               initialization = 1; // avoids recursion
>               try {
>                 // do stuff which might recurse on
>                 // ensureLogManagerInitialized()
>               } finally {
>                 initialization = 2; // mark as done
>               }
>             }
>         }
>
>         vs 2)
>
>         volatile boolean initializationDone = false;
>         boolean initializationCalled = false;
>
>         ensureLogManagerInitialized() {
>             if (initializationDone) return; // already done
>             synchronized(this) {
>               if (initializedCalled || initializationDone) return;
>               initializedCalled = true; // avoids recursion
>               try {
>                 // do stuff which might recurse on
>                 // ensureLogManagerInitialized()
>               } finally {
>                   initializationDone = true; // mark as done.
>               }
>             }
>         }
>
>         Don't know if you have a preference for one or the other.
>         My current impl (still under test) is 2)
>
>         best regards,
>
>         -- daniel
>
>
>                 I can make rootLogger volatile. I'm not 100% sure it is
>                 required - but
>                 I think
>                 it is more correct, and as such it will make the code
>                 more maintainable.
>                 Good point. It's probably  something I would have
>                 overlooked.
>
>
>
>             I think volatile rootLogger field is required for correct
>             publication of
>             RootLogger instances if double-checked locking is used...
>
>
>             Regards, Peter
>
>
>
>