Re: 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.fu...@oracle.com
<mailto:daniel.fu...@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