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

[Daniel Fuchs]

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

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