On 03/27/2015 12:27 PM, Daniel Fuchs wrote:
On 26/03/15 15:18, Peter Levart wrote:
Hi Daniel,
...or, if you keep CHM, you might be able to use it for a benefit.
You say that retrieving a logger by name is the most frequent
operation.
If you can prove that moving a statement in addLocalLogger method that
publishes the Logger via CHM to the end of synchronized block without
hurting the logic that exists in-between and initializes the Logger
instance, then you can implement the findLogger method in a way that
almost never needs to enter synchronized block:
http://cr.openjdk.java.net/~plevart/jdk9-dev/LogManager.CHM/webrev.01/
Right. I was also thinking that there may be a way to
use computeIfAbsent to rewrite some of the old logic - but I'd
rather do that in a separate change set.
This is partly why I didn't want to go much more beyond
the simple switch here. I'll keep your idea above in mind though.
Synchronization in LogManager is something that has always proved
to require careful thinking ;-)
Is it OK with you if I log a follow up RFE instead?
Yes, by all means.
CHM.computeIfAbsent() does use a lock, but it's a lock on a hash-bin, so
it's tricky if your logic is re-entrant (CHM.computIfAbsent is not
reentrant). Just exposing a get() without synchronization like I
proposed in findLogger() should be pretty safe if you do the publication
of new Logger instance as the last action in addLocalLogger synchronized
block. Those methods that change state in the context are better left
synchronized on the context instance as they must do several mutations
atomically involving various kinds of objects (Nodes, Loggers) that are
interconnected. And you don't loose much by synchronizing as creating
new Loggers is not a frequent operation and you can keep the logic plain
and simple. I think it's only worth optimizing for common case (the
findLogger()).
Here is a new webrev - with Peter's suggestion implemented.
I also made a few tweaks to the test.
http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.01/
best regards,
-- daniel
Hi Daniel,
This looks good.
Regarding what is most frequent use of Logger, typical use pattern is:
public class SomeClass {
private static final Logger log =
Logger.getLogger(SomeClass.class.getName());
...so in this case findLogger() is only called once, returns null and
Logger is then created within a synchronized block. What we did here
does not make much difference in this case. Loggers are numbered in
quantities that are similar to # of classes: 10s, 100s, 1000s, not much
more. So synchronization is only relevant for start-up performance.
But I've seen code like this too:
Logger.getLogger(SomeClass.class.getName()).info(...);
This code was not very scalable before, but with this patch it is.
Regards, Peter
Regards, Peter
best regards
-- daniel
Regards, Peter
https://bugs.openjdk.java.net/browse/JDK-7113878
http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00
best regards,
-- daniel
