Optimize logger #3370
Optimize logger #3370
Conversation
|
Some of these changes might even benefit from being cherry picked to the current released versions |
| foreach (var item in oldLoggerCache ?? Enumerable.Empty<KeyValuePair<Type, Lazy<Logger>>>()) | ||
| { | ||
| var lazyLogger = item.Value; | ||
| if (lazyLogger.IsValueCreated) | ||
| { | ||
| lazyLogger.Value.Unregister(); | ||
| } |
There was a problem hiding this comment.
I have realized the config change handler is never unregistered and effectively the old logger instances can still be rooted.
| @@ -80,39 +77,76 @@ private void ConfigureLoggers() | |||
| il.IsEnabled = (logging & LoggingOptions.Console) == LoggingOptions.Console; | |||
| if (il is InternalSystemDiagnosticsLogger) | |||
| il.IsEnabled = (logging & LoggingOptions.SystemDiagnostics) == LoggingOptions.SystemDiagnostics; | |||
|
|
|||
| if (!IsEnabled) | |||
There was a problem hiding this comment.
For all of these properties the last InternalLogger will overwrite the value on the Logger. If I understand the purpose the properties on the Logger should be true if any of the InternalLogger are true.
There was a problem hiding this comment.
I do suck at boolean logic and would probably fail any job interview 👯♂️
Isn't that exactly what it is doing? If the property is false it will check the property of the internal logger and set it to the corresponding value of the internal logger. If the internal logger is true it sets the property to true and subsequent checks don't need to look at the internal logger value anymore. If the property gets set to false the next iteration needs to check the value of the next internal logger. So if all are false it remains false. If one is true it will turn to true and subsequent checks omit toggling it. I wanted to avoid Any()
danielmarbach
force-pushed
the
logger-check-enabled-on-hotpath
branch
from
e670efd
to
145021c
Compare
|
@dscpinheiro I think this is ready for review. See also my answer to @normj |
|
Ping @normj @dscpinheiro |
|
Is there anything else required here to be able to move this forward? |
Description
This PR removes the global lock on the logger and introduces currently internal properties to check on the hot path whether the logger is enabled to avoid resource intensive operations for log levels that are not enabled.
Motivation and Context
When requests are canonicalized (is that even a word :D) the resource path runs for the canonicalization process which then acquires a logger. This acquires a global lock to then access a dictionary that quite likely already contains the logger. Furthermore it always allocates strings for debug logging that might not be required when that log level is not even requested.
Testing
Screenshots (if appropriate)
Types of changes
Checklist
License