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

[Daniel Fuchs]

On 26/03/15 15:18, Peter Levart wrote:
> 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()).


Hi Peter,

OK - you almost convinced me. I'm experimenting your idea.
There are a few tricky parts - such as moving the publication
of the logger to the last statement in addLocalLogger, but
fortunately some of the existing tests catch that if you
do it wrong ;-)

That said, the 'common' case is - I think - to call
Logger.getLogger() only once when you create the logger.
So in that common case, findLogger finds nothing - but then
addLocalLogger is called and so synchronization still has to
happen.

I'll send an updated webrev if I see that tests on all platform
succeed. We can then decide whether that's worth it :-)

Thanks for the reviews and ideas!

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