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 <[email protected]
<mailto:[email protected]>> 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
