From a78897cdd7276023c2fd76a5886be53a19ad2956 Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Fri, 7 May 2021 13:24:37 +0200 Subject: [PATCH 1/9] fixes(configuration): Not unique exporter for exporter type --- .../Configs/ImmutableConfigBuilder.cs | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs index d89e71868d..fd60a52dcd 100644 --- a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs +++ b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs @@ -90,16 +90,18 @@ private static ImmutableHashSet GetDiagnosers(IEnumerable GetExporters(IEnumerable exporters, ImmutableHashSet uniqueDiagnosers) { - var result = new List(); + + var mergeDictionary = new Dictionary(); foreach (var exporter in exporters) - if (!result.Contains(exporter)) - result.Add(exporter); + mergeDictionary[exporter.GetType()] = exporter; foreach (var diagnoser in uniqueDiagnosers) - foreach (var exporter in diagnoser.Exporters) - if (!result.Contains(exporter)) - result.Add(exporter); + foreach (var exporter in diagnoser.Exporters) + mergeDictionary[exporter.GetType()] = exporter; + + var result = mergeDictionary.Values.ToList(); + var hardwareCounterDiagnoser = uniqueDiagnosers.OfType().SingleOrDefault(); var disassemblyDiagnoser = uniqueDiagnosers.OfType().SingleOrDefault(); @@ -111,6 +113,27 @@ private static ImmutableArray GetExporters(IEnumerable exp for (int i = result.Count - 1; i >=0; i--) if (result[i] is IExporterDependencies exporterDependencies) foreach (var dependency in exporterDependencies.Dependencies) + /* + * I don't know how to correct + * the problem is: + * you have banchmark like this: + * + * // Global Current Culture separetor is Semicolon; + * [CsvMeasurementsExporter(CsvSeparator.Comma)] // force use Comma + * [RPlotExporter] + * public class MYBanch + * { + * + * } + * + * RPlotExporter is depend from CsvMeasurementsExporter.Default + * + * If I check for the type of the exporter RPlotExporter will fail + * because it will find an unexpected delimiter, + * if I stay the check as it is + * RPlotExporter it will work but the generated CSV + * will not have the required delimiter. + */ if (!result.Contains(dependency)) result.Insert(i, dependency); // All the exporter dependencies should be added before the exporter From 36f5df688dbf6ef449d4639c866d6bed133686e8 Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Fri, 7 May 2021 14:43:15 +0200 Subject: [PATCH 2/9] fixes(configuration): Assembly Config Attribute Priorty Assembly-level configuration attributes must have a lower priority than class to allow overriding their behavior. --- src/BenchmarkDotNet/Running/BenchmarkConverter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BenchmarkDotNet/Running/BenchmarkConverter.cs b/src/BenchmarkDotNet/Running/BenchmarkConverter.cs index fdca381fc8..2ed65be581 100644 --- a/src/BenchmarkDotNet/Running/BenchmarkConverter.cs +++ b/src/BenchmarkDotNet/Running/BenchmarkConverter.cs @@ -79,7 +79,7 @@ private static ImmutableConfig GetFullTypeConfig(Type type, IConfig config) var typeAttributes = type.GetCustomAttributes(true).OfType(); var assemblyAttributes = type.Assembly.GetCustomAttributes().OfType(); - foreach (var configFromAttribute in typeAttributes.Concat(assemblyAttributes)) + foreach (var configFromAttribute in assemblyAttributes.Concat(typeAttributes)) config = ManualConfig.Union(config, configFromAttribute.Config); return ImmutableConfigBuilder.Create(config); From d50db209541d9cff3213a0a54429524d1e72c1dd Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Sat, 15 May 2021 12:24:13 +0200 Subject: [PATCH 3/9] feat(Logger): Added LogKind.Warning --- src/BenchmarkDotNet/Loggers/LogKind.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BenchmarkDotNet/Loggers/LogKind.cs b/src/BenchmarkDotNet/Loggers/LogKind.cs index 8f3dff72a8..8fa4660aa8 100644 --- a/src/BenchmarkDotNet/Loggers/LogKind.cs +++ b/src/BenchmarkDotNet/Loggers/LogKind.cs @@ -2,6 +2,6 @@ { public enum LogKind { - Default, Help, Header, Result, Statistic, Info, Error, Hint + Default, Help, Header, Result, Statistic, Info, Error, Hint, Warning } } \ No newline at end of file From 2afee01ac44741f9aa9ea00bb43fc7eeb060828a Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Sat, 15 May 2021 12:25:58 +0200 Subject: [PATCH 4/9] feat(Logger): ConsoleLogger added LogKind.Warning ColorSheme --- src/BenchmarkDotNet/Loggers/ConsoleLogger.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/BenchmarkDotNet/Loggers/ConsoleLogger.cs b/src/BenchmarkDotNet/Loggers/ConsoleLogger.cs index 8d9a96e939..0a9520f964 100644 --- a/src/BenchmarkDotNet/Loggers/ConsoleLogger.cs +++ b/src/BenchmarkDotNet/Loggers/ConsoleLogger.cs @@ -71,7 +71,8 @@ private static Dictionary CreateColorfulScheme() => { LogKind.Statistic, ConsoleColor.Cyan }, { LogKind.Info, ConsoleColor.DarkYellow }, { LogKind.Error, ConsoleColor.Red }, - { LogKind.Hint, ConsoleColor.DarkCyan } + { LogKind.Hint, ConsoleColor.DarkCyan }, + { LogKind.Warning, ConsoleColor.Yellow } }; [PublicAPI] From 0a3ac7c93835cfdad47751c0201335bc02a93645 Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Sat, 15 May 2021 12:30:57 +0200 Subject: [PATCH 5/9] feat(Logger): LinqPadLogger added LogKind.Warning ColorSheme --- src/BenchmarkDotNet/Loggers/LinqPadLogger.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/BenchmarkDotNet/Loggers/LinqPadLogger.cs b/src/BenchmarkDotNet/Loggers/LinqPadLogger.cs index 930d19d4ca..d4bb2f873f 100644 --- a/src/BenchmarkDotNet/Loggers/LinqPadLogger.cs +++ b/src/BenchmarkDotNet/Loggers/LinqPadLogger.cs @@ -82,7 +82,8 @@ private static IReadOnlyDictionary CreateDarkScheme() => { LogKind.Statistic, "#00FFFF" }, { LogKind.Info, "#808000" }, { LogKind.Error, "#FF0000" }, - { LogKind.Hint, "#008080" } + { LogKind.Hint, "#008080" }, + { LogKind.Warning, "#FFFF00" }, }; private static IReadOnlyDictionary CreateLightScheme() => @@ -95,7 +96,8 @@ private static IReadOnlyDictionary CreateLightScheme() => { LogKind.Statistic, "#008080" }, { LogKind.Info, "#808000" }, { LogKind.Error, "#FF0000" }, - { LogKind.Hint, "#008080" } + { LogKind.Hint, "#008080" }, + { LogKind.Warning, "#FFFF00" }, }; } } \ No newline at end of file From ffa58883fd21c71a60ece0a066b1ddfe3f40b4cc Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Sat, 15 May 2021 12:52:27 +0200 Subject: [PATCH 6/9] feat(configuration): Print warning when dependency of export is already present in the configuration. --- .../Configs/ImmutableConfigBuilder.cs | 44 +++++++++++++------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs index fd60a52dcd..9f6969296e 100644 --- a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs +++ b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs @@ -38,7 +38,7 @@ public static ImmutableConfig Create(IConfig source) var uniqueHardwareCounters = source.GetHardwareCounters().ToImmutableHashSet(); var uniqueDiagnosers = GetDiagnosers(source.GetDiagnosers(), uniqueHardwareCounters); - var uniqueExporters = GetExporters(source.GetExporters(), uniqueDiagnosers); + var uniqueExporters = GetExporters(source.GetExporters(), uniqueDiagnosers, uniqueLoggers); var uniqueAnalyzers = GetAnalysers(source.GetAnalysers(), uniqueDiagnosers); var uniqueValidators = GetValidators(source.GetValidators(), MandatoryValidators, source.Options); @@ -88,7 +88,9 @@ private static ImmutableHashSet GetDiagnosers(IEnumerable GetExporters(IEnumerable exporters, ImmutableHashSet uniqueDiagnosers) + private static ImmutableArray GetExporters(IEnumerable exporters, + ImmutableHashSet uniqueDiagnosers, + IEnumerable loggers = null) { var mergeDictionary = new Dictionary(); @@ -114,28 +116,42 @@ private static ImmutableArray GetExporters(IEnumerable exp if (result[i] is IExporterDependencies exporterDependencies) foreach (var dependency in exporterDependencies.Dependencies) /* - * I don't know how to correct - * the problem is: - * you have banchmark like this: + * When exporter that depends on an other already present in the configuration print warning. * - * // Global Current Culture separetor is Semicolon; + * Example: + * + * // Global Current Culture separator is Semicolon; * [CsvMeasurementsExporter(CsvSeparator.Comma)] // force use Comma * [RPlotExporter] - * public class MYBanch + * public class MyBanch * { * * } * * RPlotExporter is depend from CsvMeasurementsExporter.Default * - * If I check for the type of the exporter RPlotExporter will fail - * because it will find an unexpected delimiter, - * if I stay the check as it is - * RPlotExporter it will work but the generated CSV - * will not have the required delimiter. - */ - if (!result.Contains(dependency)) + * On active logger will by print: + * "The CsvMeasurementsExporter is already present in the configuration. There may be unexpected results of RPlotExporter. + * + */ + if (!result.Any(exporter=> exporter.GetType() == dependency.GetType())) result.Insert(i, dependency); // All the exporter dependencies should be added before the exporter + else + { + if (loggers?.Any() == true) + { + var warning = $"The {dependency.Name} is already present in the configuration. There may be unexpected results of {result[i].GetType().Name}."; + foreach (var logger in loggers) + { + logger.WriteLine(Loggers.LogKind.Warning, + warning); + } + } + else + { + // If there are no loggers should I throw an exception? + } + } result.Sort((left, right) => (left is IExporterDependencies).CompareTo(right is IExporterDependencies)); // the case when they were defined by user in wrong order ;) From f075ec58d752150b53b4f958932862956f455cc4 Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Mon, 17 May 2021 18:21:12 +0200 Subject: [PATCH 7/9] test: add test to check generation of warning when exporter dependency already exist in configuration --- .../Configs/ImmutableConfigTests.cs | 42 +++++++++++++++++++ .../Loggers/DelegateLogger.cs | 28 +++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 tests/BenchmarkDotNet.Tests/Loggers/DelegateLogger.cs diff --git a/tests/BenchmarkDotNet.Tests/Configs/ImmutableConfigTests.cs b/tests/BenchmarkDotNet.Tests/Configs/ImmutableConfigTests.cs index df068ac504..566618e180 100644 --- a/tests/BenchmarkDotNet.Tests/Configs/ImmutableConfigTests.cs +++ b/tests/BenchmarkDotNet.Tests/Configs/ImmutableConfigTests.cs @@ -331,5 +331,47 @@ public class TestExporterDependency : IExporter public string Name => nameof(TestExporterDependency); public void ExportToLog(Summary summary, ILogger logger) { } } + + [Fact] + public void GenerateWarningWhenExporterDependencyAlreadyExistInConfig() + { + System.Globalization.CultureInfo currentCulture = default; + System.Globalization.CultureInfo currentUICulture = default; + { + var ct = System.Threading.Thread.CurrentThread; + currentCulture = ct.CurrentCulture; + currentUICulture = ct.CurrentUICulture; + ct.CurrentCulture = System.Globalization.CultureInfo.InvariantCulture; + ct.CurrentUICulture = System.Globalization.CultureInfo.InvariantCulture; + } + try + { + var countWarning = 0; + var logger = new Loggers.DelegateLogger((kind, text) => + { + if (kind == LogKind.Warning) + { + countWarning++; + } + }); + + var mutable = ManualConfig.CreateEmpty(); + mutable.AddLogger(logger); + mutable.AddExporter(new BenchmarkDotNet.Exporters.Csv.CsvMeasurementsExporter(BenchmarkDotNet.Exporters.Csv.CsvSeparator.Comma)); + mutable.AddExporter(RPlotExporter.Default); + + var final = ImmutableConfigBuilder.Create(mutable); + + Assert.Equal(1, countWarning); + } + finally + { + var ct = System.Threading.Thread.CurrentThread; + ct.CurrentCulture = currentCulture; + ct.CurrentUICulture = currentUICulture; + + } + + } } } diff --git a/tests/BenchmarkDotNet.Tests/Loggers/DelegateLogger.cs b/tests/BenchmarkDotNet.Tests/Loggers/DelegateLogger.cs new file mode 100644 index 0000000000..5d48c982a9 --- /dev/null +++ b/tests/BenchmarkDotNet.Tests/Loggers/DelegateLogger.cs @@ -0,0 +1,28 @@ +using BenchmarkDotNet.Loggers; +using System; + +namespace BenchmarkDotNet.Tests.Loggers +{ + internal class DelegateLogger : ILogger + { + private readonly Action writeAction; + + public DelegateLogger(Action writeAction) + { + this.writeAction = writeAction; + } + + public string Id { get; } + public int Priority => 0; + + public virtual void Write(LogKind logKind, string text) + => writeAction?.Invoke(logKind, text); + + public virtual void WriteLine() { } + + public virtual void WriteLine(LogKind logKind, string text) + => writeAction?.Invoke(logKind, string.Empty); + + public void Flush() { } + } +} From bfc263f833e4be605ae8bdee95d45abfc38731fe Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Mon, 24 May 2021 10:00:20 +0200 Subject: [PATCH 8/9] feat: Added LoggerAnalyzer LoggerAnalyzer, allow to show a assertion on the summary when in the benchmark there is some warnings or errors. --- .../Analysers/LoggerAnalyzer.cs | 56 +++++++++++++++++++ src/BenchmarkDotNet/Configs/DefaultConfig.cs | 2 + 2 files changed, 58 insertions(+) create mode 100644 src/BenchmarkDotNet/Analysers/LoggerAnalyzer.cs diff --git a/src/BenchmarkDotNet/Analysers/LoggerAnalyzer.cs b/src/BenchmarkDotNet/Analysers/LoggerAnalyzer.cs new file mode 100644 index 0000000000..0f4328a8e8 --- /dev/null +++ b/src/BenchmarkDotNet/Analysers/LoggerAnalyzer.cs @@ -0,0 +1,56 @@ +using BenchmarkDotNet.Loggers; +using BenchmarkDotNet.Reports; +using System; +using System.Collections.Generic; + +namespace BenchmarkDotNet.Analysers +{ + public class LoggerAnalyzer : IAnalyser, ILogger + { + private int errorCount; + private int warningCount; + public static readonly LoggerAnalyzer Instance = new LoggerAnalyzer(); + + private LoggerAnalyzer() + { + + } + + public string Id => nameof(LoggerAnalyzer); + + public int Priority => throw new NotImplementedException(); + + public IEnumerable Analyse(Summary summary) + { + if (errorCount > 0) + { + yield return Conclusion.CreateError(Id, "There is one or more errors. See log for dettails."); + } + if (warningCount > 0) + { + yield return Conclusion.CreateWarning(Id, "There is one or more warning. See log for details."); + } + } + + public void Flush() { } + + public void Write(LogKind logKind, string text) + { + switch (logKind) + { + case LogKind.Error: + errorCount++; + break; + case LogKind.Warning: + warningCount++; + break; + default: + break; + } + } + + public void WriteLine() { } + + public void WriteLine(LogKind logKind, string text) => Write(logKind, text); + } +} diff --git a/src/BenchmarkDotNet/Configs/DefaultConfig.cs b/src/BenchmarkDotNet/Configs/DefaultConfig.cs index eb3fb6e80a..c3fc6e9d87 100644 --- a/src/BenchmarkDotNet/Configs/DefaultConfig.cs +++ b/src/BenchmarkDotNet/Configs/DefaultConfig.cs @@ -42,6 +42,7 @@ public IEnumerable GetLoggers() yield return LinqPadLogger.Instance; else yield return ConsoleLogger.Default; + yield return LoggerAnalyzer.Instance; } public IEnumerable GetAnalysers() @@ -53,6 +54,7 @@ public IEnumerable GetAnalysers() yield return RuntimeErrorAnalyser.Default; yield return ZeroMeasurementAnalyser.Default; yield return BaselineCustomAnalyzer.Default; + yield return LoggerAnalyzer.Instance; } public IEnumerable GetValidators() From f172a3f8f9cb6ffd0978f0427885e393ca732b19 Mon Sep 17 00:00:00 2001 From: Giuseppe Lippolis Date: Mon, 24 May 2021 10:22:46 +0200 Subject: [PATCH 9/9] feat(configuration): Adding a warning when the exporter will be overridden. --- .../Configs/ImmutableConfigBuilder.cs | 45 ++++++++++++------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs index 9f6969296e..4343709f72 100644 --- a/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs +++ b/src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs @@ -93,14 +93,41 @@ private static ImmutableArray GetExporters(IEnumerable exp IEnumerable loggers = null) { + void PrintWarning(string message) + { + if (loggers?.Any() == true) + { + foreach (var logger in loggers) + { + logger.WriteLine(Loggers.LogKind.Warning, + message); + } + } + } + var mergeDictionary = new Dictionary(); foreach (var exporter in exporters) - mergeDictionary[exporter.GetType()] = exporter; + { + var exporterType = exporter.GetType(); + if (mergeDictionary.ContainsKey(exporterType)) + { + PrintWarning($"The exporter {exporterType} is already present in configuration. There may be unexpected results."); + } + mergeDictionary[exporterType] = exporter; + } + foreach (var diagnoser in uniqueDiagnosers) foreach (var exporter in diagnoser.Exporters) - mergeDictionary[exporter.GetType()] = exporter; + { + var exporterType = exporter.GetType(); + if (mergeDictionary.ContainsKey(exporterType)) + { + PrintWarning($"The exporter {exporterType} of {diagnoser.GetType().Name} is already present in configuration. There may be unexpected results."); + } + mergeDictionary[exporterType] = exporter; + }; var result = mergeDictionary.Values.ToList(); @@ -138,19 +165,7 @@ private static ImmutableArray GetExporters(IEnumerable exp result.Insert(i, dependency); // All the exporter dependencies should be added before the exporter else { - if (loggers?.Any() == true) - { - var warning = $"The {dependency.Name} is already present in the configuration. There may be unexpected results of {result[i].GetType().Name}."; - foreach (var logger in loggers) - { - logger.WriteLine(Loggers.LogKind.Warning, - warning); - } - } - else - { - // If there are no loggers should I throw an exception? - } + PrintWarning($"The {dependency.Name} is already present in the configuration. There may be unexpected results of {result[i].GetType().Name}."); } result.Sort((left, right) => (left is IExporterDependencies).CompareTo(right is IExporterDependencies)); // the case when they were defined by user in wrong order ;)