Minimize lock usage in InternalLoggerRegistry

[ppkarwasz]

It has been reported that holding a lock on InternalLoggerRegistry during the creation of a logger can cause performance loss and deadlocks. The logger constructor can trigger property lookups and other pluggable operations, we don't entirely control. The fix to #3252 only covered one of these cases.

This change moves the instantiation of new Loggers outside the write lock. While in some cases, this will cause multiple instantiations of loggers with the same parameters, all such loggers are functionally equivalent. On the other hand, the change allows the creation of different loggers in parallel.

Closes #3399

[vy]

Are you sure this is necessary? If WR returns a non-null value, and you hold a reference to that, I don't think WR can be emptied in the meantime. Am I mistaken?

[ppkarwasz]

You might be right, but I am not eager to risk another 2.24.1 😄

My reasoning was:

• We get a WR to newLogger.
• At that point in code newLogger has already been referenced for the last time. The () -> new WeakReference<>(newLogger) has already been used and can be reclaimed.
• If a GC occurs just before the WR.get(), the reference might be emptied.

Since I reference newLogger on line 186, currentLogger will never be null.

[vy]

At that point in code newLogger has already been referenced for the last time. The () -> new WeakReference<>(newLogger) has already been used and can be reclaimed.

No, it cannot; because there is still a strong reference to the instance.

You might be right, but I am not eager to risk another 2.24.1 😄

Could you at least document that this is a "speculative measure"?

[vy]

This logic is wrong. It can be that there is a WR registered for the (messageFactory, name) key, yet the WRs reference is reclaimed. I think you need to revert this block back to my LoggerRegistry::computeIfAbsent implementation in 2062676:

Map<String, WeakReference<Logger>> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory);
// noinspection Java8MapApi (avoid the allocation of lambda passed to `Map::computeIfAbsent`)
if (loggerRefByName == null) {
    loggerRefByNameByMessageFactory.put(messageFactory, loggerRefByName = new HashMap<>());
}
final WeakReference<Logger> loggerRef = loggerRefByName.get(name);
if (loggerRef == null || (logger = loggerRef.get()) == null) {
    loggerRefByName.put(name, new WeakReference<>(logger = newLogger));
}
return logger;