-
Notifications
You must be signed in to change notification settings - Fork 863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize logger #3370
Open
danielmarbach
wants to merge
8
commits into
aws:v4-development
Choose a base branch
from
danielmarbach:logger-check-enabled-on-hotpath
base: v4-development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Optimize logger #3370
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
e41a051
If one of the loggers is enabled the entire logger abstraction is ena…
danielmarbach 0966832
Do not allocate debug information when not necessary
danielmarbach 8cb487f
Remove global logger lock
danielmarbach 553409f
Cleanup
danielmarbach 282ec19
Satisfy null constraint eventhough this path cannot be hit
danielmarbach b19893f
AutoProperty
danielmarbach d97d1e8
use exact size
danielmarbach ffca58c
Remove unnecessary locals
danielmarbach File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,14 +13,11 @@ | |
* permissions and limitations under the License. | ||
*/ | ||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Collections.Specialized; | ||
using System.Diagnostics; | ||
using System.Globalization; | ||
using System.Reflection; | ||
using System.Text; | ||
using System.ComponentModel; | ||
using Amazon.Runtime; | ||
using System.Linq; | ||
using System.Threading; | ||
using Amazon.Util.Internal; | ||
|
||
namespace Amazon.Runtime.Internal.Util | ||
|
@@ -31,9 +28,8 @@ namespace Amazon.Runtime.Internal.Util | |
/// </summary> | ||
public class Logger : ILogger | ||
{ | ||
private static IDictionary<Type, Logger> cachedLoggers = new Dictionary<Type, Logger>(); | ||
private static ConcurrentDictionary<Type, Lazy<Logger>> cachedLoggers = new ConcurrentDictionary<Type, Lazy<Logger>>(); | ||
private List<InternalLogger> loggers; | ||
private static Logger emptyLogger = new Logger(); | ||
|
||
private Logger() | ||
{ | ||
|
@@ -46,19 +42,20 @@ private Logger() | |
#endif | ||
private Logger(Type type) | ||
{ | ||
loggers = new List<InternalLogger>(); | ||
|
||
if(!InternalSDKUtils.IsRunningNativeAot()) | ||
{ | ||
loggers = new List<InternalLogger>(3); | ||
#pragma warning disable | ||
InternalLog4netLogger log4netLogger = new InternalLog4netLogger(type); | ||
loggers.Add(log4netLogger); | ||
loggers.Add(new InternalLog4netLogger(type)); | ||
#pragma warning restore | ||
} | ||
else | ||
{ | ||
loggers = new List<InternalLogger>(2); | ||
} | ||
|
||
loggers.Add(new InternalConsoleLogger(type)); | ||
InternalSystemDiagnosticsLogger sdLogger = new InternalSystemDiagnosticsLogger(type); | ||
loggers.Add(sdLogger); | ||
loggers.Add(new InternalSystemDiagnosticsLogger(type)); | ||
ConfigureLoggers(); | ||
AWSConfigs.PropertyChanged += ConfigsChanged; | ||
} | ||
|
@@ -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) | ||
{ | ||
IsEnabled = il.IsEnabled; | ||
} | ||
if (!IsErrorEnabled) | ||
{ | ||
IsErrorEnabled = il.IsErrorEnabled; | ||
} | ||
if (!IsInfoEnabled) | ||
{ | ||
IsInfoEnabled = il.IsInfoEnabled; | ||
} | ||
if (!IsDebugEnabled) | ||
{ | ||
IsDebugEnabled = il.IsDebugEnabled; | ||
} | ||
} | ||
} | ||
|
||
#region Static accessor | ||
|
||
public static Logger GetLogger(Type type) | ||
{ | ||
if (type == null) throw new ArgumentNullException("type"); | ||
if (type == null) throw new ArgumentNullException(nameof(type)); | ||
|
||
Logger l; | ||
lock (cachedLoggers) | ||
{ | ||
if (!cachedLoggers.TryGetValue(type, out l)) | ||
{ | ||
l = new Logger(type); | ||
cachedLoggers[type] = l; | ||
} | ||
} | ||
return l; | ||
// Use a lazy initialization to ensure that we only create a logger for a given type once because | ||
// the constructor does some heavy lifting including setting up event listeners. | ||
var lazyLogger = cachedLoggers.GetOrAdd(type, static t => new Lazy<Logger>(() => new Logger(t))); | ||
return lazyLogger.Value; | ||
} | ||
|
||
public static void ClearLoggerCache() | ||
{ | ||
lock (cachedLoggers) | ||
var newLoggerCache = new ConcurrentDictionary<Type, Lazy<Logger>>(); | ||
ConcurrentDictionary<Type, Lazy<Logger>> oldLoggerCache; | ||
|
||
do | ||
{ | ||
cachedLoggers = new Dictionary<Type, Logger>(); | ||
oldLoggerCache = cachedLoggers; | ||
} while (Interlocked.CompareExchange(ref cachedLoggers, newLoggerCache, oldLoggerCache) != oldLoggerCache); | ||
|
||
// Unregister all the loggers in the old cache | ||
foreach (var item in oldLoggerCache ?? Enumerable.Empty<KeyValuePair<Type, Lazy<Logger>>>()) | ||
{ | ||
var lazyLogger = item.Value; | ||
if (lazyLogger.IsValueCreated) | ||
{ | ||
lazyLogger.Value.Unregister(); | ||
} | ||
Comment on lines
+123
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have realized the config change handler is never unregistered and effectively the old logger instances can still be rooted. |
||
} | ||
} | ||
|
||
public static Logger EmptyLogger { get { return emptyLogger; } } | ||
public static Logger EmptyLogger { get; } = new Logger(); | ||
|
||
#endregion | ||
|
||
internal bool IsEnabled { get; private set; } | ||
|
||
internal virtual bool IsErrorEnabled { get; private set; } | ||
|
||
internal virtual bool IsDebugEnabled { get; private set; } | ||
|
||
internal virtual bool IsInfoEnabled { get; private set; } | ||
|
||
internal void Unregister() | ||
{ | ||
AWSConfigs.PropertyChanged -= ConfigsChanged; | ||
} | ||
|
||
#region Logging methods | ||
|
||
public void Flush() | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all of these properties the last
InternalLogger
will overwrite the value on theLogger
. If I understand the purpose the properties on theLogger
should be true if any of theInternalLogger
are true.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
https://dotnetfiddle.net/Se39FX