RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Peter Levart]

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