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

[Peter Levart]

On 03/26/2015 02:55 PM, Daniel Fuchs wrote:
> On 26/03/15 14:43, Peter Levart wrote:
>> On 03/26/2015 01:28 PM, David Holmes wrote:
>>> Hi Daniel,
>>>
>>> On 26/03/2015 10:08 PM, Daniel Fuchs wrote:
>>>> Please find below a trivial fix for
>>>>
>>>>
>>>> 7113878: LogManager - namedLoggers should be ConcurrentHashMap
>>>>           instead of Hashtable
>>>
>>> As you say in the bug report, now that the map is always accessed
>>> within synchronized code this serves no purpose. The map not only
>>> doesn't need to be concurrent, it doesn't even need to be thread-safe!
>>> So why not replace with a simple HashMap?
>>>
>>> David
>>
>> 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()).

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