Skip to content

Commit aea32df

Browse files
timcasselltimcassell
committed
PR comments
1 parent ea607f5 commit aea32df

File tree

13 files changed

+92
-89
lines changed

13 files changed

+92
-89
lines changed

‎src/BenchmarkDotNet/Code/CodeGenerator.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ internal static string Generate(BuildPartition buildPartition)
6666
.Replace("$MeasureExtraStats$", buildInfo.Config.HasExtraStatsDiagnoser() ? "true" : "false")
6767
.Replace("$DisassemblerEntryMethodName$", DisassemblerConstants.DisassemblerEntryMethodName)
6868
.Replace("$WorkloadMethodCall$", provider.GetWorkloadMethodCall(passArguments))
69-
.Replace("$InProcessDiagnosers$", string.Join($",\n", buildInfo.CompositeInProcessDiagnoser.GetHandlersSourceCode(benchmark)))
69+
.Replace("$InProcessDiagnosers$", string.Join($",\n", buildInfo.CompositeInProcessDiagnoser.GetSourceCode(benchmark)))
7070
.RemoveRedundantIfDefines(compilationId);
7171

7272
benchmarkTypeCode = Unroll(benchmarkTypeCode, benchmark.Job.ResolveValue(RunMode.UnrollFactorCharacteristic, EnvironmentResolver.Instance));

‎src/BenchmarkDotNet/Diagnosers/CompositeDiagnoser.cs‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -63,23 +63,22 @@ public sealed class CompositeInProcessDiagnoser(IReadOnlyList<IInProcessDiagnose
6363
public const string HeaderKey = "// InProcessDiagnoser";
6464
public const string ResultsKey = $"{HeaderKey}Results";
6565

66-
public IEnumerable<string> GetHandlersSourceCode(BenchmarkCase benchmarkCase)
67-
=> inProcessDiagnosers
68-
.Select((d, i) => d.GetHandlerSourceCode(benchmarkCase, i))
69-
.Where(s => !string.IsNullOrEmpty(s));
66+
public IEnumerable<string> GetSourceCode(BenchmarkCase benchmarkCase)
67+
=> GetInProcessDiagnoserRouters(benchmarkCase)
68+
.Select(router => router.ToSourceCode());
7069

71-
public IReadOnlyList<IInProcessDiagnoserHandler> GetInProcessHandlers(BenchmarkCase benchmarkCase)
72-
=> [.. inProcessDiagnosers
73-
.Select((d, i) => d.GetHandler(benchmarkCase, i))
74-
.WhereNotNull()];
70+
public IEnumerable<InProcessDiagnoserRouter> GetInProcessDiagnoserRouters(BenchmarkCase benchmarkCase)
71+
=> inProcessDiagnosers
72+
.Select((d, i) => new InProcessDiagnoserRouter() { index = i, runMode = d.GetRunMode(benchmarkCase), handler = d.GetHandler(benchmarkCase) })
73+
.Where(router => router.handler != null);
7574

7675
public void DeserializeResults(int index, BenchmarkCase benchmarkCase, string results)
7776
=> inProcessDiagnosers[index].DeserializeResults(benchmarkCase, results);
7877
}
7978

8079
[UsedImplicitly]
8180
[EditorBrowsable(EditorBrowsableState.Never)]
82-
public sealed class CompositeInProcessDiagnoserHandler(IReadOnlyList<IInProcessDiagnoserHandler> handlers, IHost host, RunMode runMode, InProcessDiagnoserActionArgs parameters)
81+
public sealed class CompositeInProcessDiagnoserHandler(IReadOnlyList<InProcessDiagnoserRouter> routers, IHost host, RunMode runMode, InProcessDiagnoserActionArgs parameters)
8382
{
8483
public void Handle(BenchmarkSignal signal)
8584
{
@@ -88,11 +87,11 @@ public void Handle(BenchmarkSignal signal)
8887
return;
8988
}
9089

91-
foreach (var handler in handlers)
90+
foreach (var router in routers)
9291
{
93-
if (handler.RunMode == runMode)
92+
if (router.runMode == runMode)
9493
{
95-
handler.Handle(signal, parameters);
94+
router.handler.Handle(signal, parameters);
9695
}
9796
}
9897

@@ -101,19 +100,19 @@ public void Handle(BenchmarkSignal signal)
101100
return;
102101
}
103102

104-
foreach (var handler in handlers)
103+
foreach (var router in routers)
105104
{
106-
if (handler.RunMode != runMode)
105+
if (router.runMode != runMode)
107106
{
108107
continue;
109108
}
110109

111-
var results = handler.SerializeResults();
110+
var results = router.handler.SerializeResults();
112111
// Send header with the diagnoser index for routing, and line count of payload (user handler may include newlines in their serialized results).
113112
// Ideally we would simply use results.Length, write it directly to host, then the host reads the exact count of chars.
114113
// But WasmExecutor does not use Broker, and reads all output, so we need to instead use line count and prepend every line with CompositeInProcessDiagnoser.ResultsKey.
115114
var resultsLines = results.Split(["\r\n", "\r", "\n"], StringSplitOptions.None);
116-
host.WriteLine($"{CompositeInProcessDiagnoser.HeaderKey} {handler.Index} {resultsLines.Length}");
115+
host.WriteLine($"{CompositeInProcessDiagnoser.HeaderKey} {router.index} {resultsLines.Length}");
117116
foreach (var line in resultsLines)
118117
{
119118
host.WriteLine($"{CompositeInProcessDiagnoser.ResultsKey} {line}");

‎src/BenchmarkDotNet/Diagnosers/IDiagnoser.cs‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,12 @@ public interface IConfigurableDiagnoser<in TConfig> : IDiagnoser
4040
public interface IInProcessDiagnoser : IDiagnoser
4141
{
4242
/// <summary>
43-
/// Gets the C# source code used to instantiate the handler in the benchmark process.
43+
/// Gets the diagnoser handler.
4444
/// </summary>
4545
/// <remarks>
46-
/// The source code must be a single expression.
46+
/// Return <see langword="null"/> to not run the diagnoser handler for the <paramref name="benchmarkCase"/>.
4747
/// </remarks>
48-
string GetHandlerSourceCode(BenchmarkCase benchmarkCase, int index);
49-
50-
/// <summary>
51-
/// Gets the handler for the same process.
52-
/// </summary>
53-
IInProcessDiagnoserHandler GetHandler(BenchmarkCase benchmarkCase, int index);
48+
IInProcessDiagnoserHandler? GetHandler(BenchmarkCase benchmarkCase);
5449

5550
/// <summary>
5651
/// Deserializes the results of the handler.
@@ -63,16 +58,6 @@ public interface IInProcessDiagnoser : IDiagnoser
6358
/// </summary>
6459
public interface IInProcessDiagnoserHandler
6560
{
66-
/// <summary>
67-
/// The index of the diagnoser.
68-
/// </summary>
69-
int Index { get; }
70-
71-
/// <summary>
72-
/// The <see cref="Diagnosers.RunMode"/> of the diagnoser for the benchmark.
73-
/// </summary>
74-
RunMode RunMode { get; }
75-
7661
/// <summary>
7762
/// Handles the signal from the benchmark.
7863
/// </summary>
@@ -82,5 +67,14 @@ public interface IInProcessDiagnoserHandler
8267
/// Serializes the results to be sent back to the host <see cref="IInProcessDiagnoser"/>.
8368
/// </summary>
8469
string SerializeResults();
70+
71+
/// <summary>
72+
/// Gets the C# source code used to instantiate the handler in the benchmark process.
73+
/// </summary>
74+
/// <remarks>
75+
/// The source code must be a single expression.
76+
/// </remarks>
77+
string ToSourceCode();
78+
8579
}
8680
}

‎src/BenchmarkDotNet/Diagnosers/InProcessDiagnoserRouter.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
using BenchmarkDotNet.Extensions;
2+
using BenchmarkDotNet.Helpers;
3+
using JetBrains.Annotations;
4+
using System.ComponentModel;
5+
6+
namespace BenchmarkDotNet.Diagnosers
7+
{
8+
[UsedImplicitly]
9+
[EditorBrowsable(EditorBrowsableState.Never)]
10+
public struct InProcessDiagnoserRouter
11+
{
12+
public IInProcessDiagnoserHandler handler;
13+
public int index;
14+
public RunMode runMode;
15+
16+
public readonly string ToSourceCode()
17+
=> $$"""
18+
new {{typeof(InProcessDiagnoserRouter).GetCorrectCSharpTypeName()}}() {
19+
{{nameof(handler)}} = {{handler.ToSourceCode()}},
20+
{{nameof(index)}} = {{index}},
21+
{{nameof(runMode)}} = {{SourceCodeHelper.ToSourceCode(runMode)}}
22+
}
23+
""";
24+
}
25+
}

‎src/BenchmarkDotNet/Disassemblers/DisassemblyDiagnoser.cs‎

Lines changed: 26 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -216,41 +216,20 @@ private static IEnumerable<IExporter> GetExporters(Dictionary<BenchmarkCase, Dis
216216
private static long SumNativeCodeSize(DisassemblyResult disassembly)
217217
=> disassembly.Methods.Sum(method => method.Maps.Sum(map => map.SourceCodes.OfType<Asm>().Sum(asm => asm.InstructionLength)));
218218

219-
string IInProcessDiagnoser.GetHandlerSourceCode(BenchmarkCase benchmarkCase, int index)
219+
IInProcessDiagnoserHandler? IInProcessDiagnoser.GetHandler(BenchmarkCase benchmarkCase)
220220
{
221-
// Mono disassembler always runs another process.
222-
if (Config.RunInHost || ShouldUseMonoDisassembler(benchmarkCase))
221+
if (GetRunMode(benchmarkCase) == RunMode.None
222+
|| Config.RunInHost
223+
// Mono disassembler always runs another process.
224+
|| ShouldUseMonoDisassembler(benchmarkCase)
225+
// We don't use handler for InProcess toolchains, the host diagnoser already handles it without needing to serialize data.
226+
|| benchmarkCase.Job.Infrastructure.TryGetToolchain(out var toolchain) && toolchain.IsInProcess)
223227
{
224228
return null;
225229
}
226-
var runMode = GetRunMode(benchmarkCase);
227-
if (runMode == RunMode.None)
228-
{
229-
return null;
230-
}
231-
232-
var clrMdArgs = BuildDisassemblerSettings(benchmarkCase, null, 0);
233-
return $$"""
234-
new {{typeof(DisassemblyDiagnoserInProcessHandler).GetCorrectCSharpTypeName()}}() {
235-
{{nameof(DisassemblyDiagnoserInProcessHandler.Index)}} = {{index}},
236-
{{nameof(DisassemblyDiagnoserInProcessHandler.RunMode)}} = {{SourceCodeHelper.ToSourceCode(runMode)}},
237-
{{nameof(DisassemblyDiagnoserInProcessHandler.ClrMdArgs)}} = new {{typeof(ClrMdArgs).GetCorrectCSharpTypeName()}}(
238-
{{typeof(Process).GetCorrectCSharpTypeName()}}.{{nameof(Process.GetCurrentProcess)}}().{{nameof(Process.Id)}},
239-
instance.GetType().FullName,
240-
{{SourceCodeHelper.ToSourceCode(clrMdArgs.MethodName)}},
241-
{{SourceCodeHelper.ToSourceCode(clrMdArgs.PrintSource)}},
242-
{{clrMdArgs.MaxDepth}},
243-
{{SourceCodeHelper.ToSourceCode(clrMdArgs.Syntax)}},
244-
{{SourceCodeHelper.ToSourceCode(clrMdArgs.TargetFrameworkMoniker)}},
245-
{{SourceCodeHelper.ToSourceCode(clrMdArgs.Filters)}}
246-
)
247-
}
248-
""";
230+
return new DisassemblyDiagnoserInProcessHandler(BuildDisassemblerSettings(benchmarkCase, null, 0));
249231
}
250232

251-
// We don't use handler for InProcess toolchains, the host diagnoser already handles it without needing to serialize data.
252-
IInProcessDiagnoserHandler IInProcessDiagnoser.GetHandler(BenchmarkCase benchmarkCase, int index) => null;
253-
254233
void IInProcessDiagnoser.DeserializeResults(BenchmarkCase benchmarkCase, string results)
255234
{
256235
var json = SimpleJsonSerializer.DeserializeObject<JsonObject>(results);
@@ -277,19 +256,15 @@ private class NativeCodeSizeMetricDescriptor : IMetricDescriptor
277256

278257
[UsedImplicitly]
279258
[EditorBrowsable(EditorBrowsableState.Never)]
280-
public sealed class DisassemblyDiagnoserInProcessHandler : IInProcessDiagnoserHandler
259+
public sealed class DisassemblyDiagnoserInProcessHandler(ClrMdArgs clrMdArgs) : IInProcessDiagnoserHandler
281260
{
282261
private DisassemblyResult _result;
283262

284-
public int Index { get; set; }
285-
public RunMode RunMode { get; set; }
286-
public ClrMdArgs ClrMdArgs { get; set; }
287-
288263
void IInProcessDiagnoserHandler.Handle(BenchmarkSignal signal, InProcessDiagnoserActionArgs parameters)
289264
{
290265
if (signal == BenchmarkSignal.AfterEngine)
291266
{
292-
_result = DisassemblyDiagnoser.GetClrMdDisassembler().AttachAndDisassemble(ClrMdArgs);
267+
_result = DisassemblyDiagnoser.GetClrMdDisassembler().AttachAndDisassemble(clrMdArgs);
293268
}
294269
}
295270

@@ -298,5 +273,21 @@ string IInProcessDiagnoserHandler.SerializeResults()
298273
SimpleJsonSerializer.CurrentJsonSerializerStrategy.Indent = false;
299274
return _result.Serialize().ToString();
300275
}
276+
277+
string IInProcessDiagnoserHandler.ToSourceCode()
278+
=> $"""
279+
new {typeof(DisassemblyDiagnoserInProcessHandler).GetCorrectCSharpTypeName()}(
280+
new {typeof(ClrMdArgs).GetCorrectCSharpTypeName()}(
281+
{typeof(Process).GetCorrectCSharpTypeName()}.{nameof(Process.GetCurrentProcess)}().{nameof(Process.Id)},
282+
instance.GetType().FullName,
283+
{SourceCodeHelper.ToSourceCode(clrMdArgs.MethodName)},
284+
{SourceCodeHelper.ToSourceCode(clrMdArgs.PrintSource)},
285+
{clrMdArgs.MaxDepth},
286+
{SourceCodeHelper.ToSourceCode(clrMdArgs.Syntax)},
287+
{SourceCodeHelper.ToSourceCode(clrMdArgs.TargetFrameworkMoniker)},
288+
{SourceCodeHelper.ToSourceCode(clrMdArgs.Filters)}
289+
)
290+
)
291+
""";
301292
}
302293
}

‎src/BenchmarkDotNet/Templates/BenchmarkType.txt‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@
2222
return;
2323

2424
BenchmarkDotNet.Diagnosers.CompositeInProcessDiagnoserHandler compositeInProcessDiagnoserHandler = new BenchmarkDotNet.Diagnosers.CompositeInProcessDiagnoserHandler(
25-
new BenchmarkDotNet.Diagnosers.IInProcessDiagnoserHandler[]
26-
{
25+
new BenchmarkDotNet.Diagnosers.InProcessDiagnoserRouter[] {
2726
$InProcessDiagnosers$
2827
},
2928
host,

‎src/BenchmarkDotNet/Toolchains/InProcess/Emit/Implementation/Runnable/RunnableReuse.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public static (Job, EngineParameters, IEngineFactory) PrepareForRun<T>(T instanc
2929
return (null, null, null);
3030

3131
var compositeInProcessDiagnoserHandler = new CompositeInProcessDiagnoserHandler(
32-
parameters.CompositeInProcessDiagnoser.GetInProcessHandlers(benchmarkCase),
32+
[..parameters.CompositeInProcessDiagnoser.GetInProcessDiagnoserRouters(benchmarkCase)],
3333
host,
3434
parameters.DiagnoserRunMode,
3535
new InProcessDiagnoserActionArgs(instance)

‎src/BenchmarkDotNet/Toolchains/InProcess/NoEmit/InProcessNoEmitRunner.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ public static void RunCore(IHost host, ExecuteParameters parameters)
132132
host.WriteLine();
133133

134134
var compositeInProcessDiagnoserHandler = new Diagnosers.CompositeInProcessDiagnoserHandler(
135-
parameters.CompositeInProcessDiagnoser.GetInProcessHandlers(benchmarkCase),
135+
[..parameters.CompositeInProcessDiagnoser.GetInProcessDiagnoserRouters(benchmarkCase)],
136136
host,
137137
parameters.DiagnoserRunMode,
138138
new Diagnosers.InProcessDiagnoserActionArgs(instance)

‎tests/BenchmarkDotNet.IntegrationTests/Diagnosers/MockInProcessDiagnoser.cs‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
using BenchmarkDotNet.Validators;
99
using BenchmarkDotNet.Extensions;
1010
using System.Collections.Generic;
11-
using BenchmarkDotNet.Helpers;
1211

1312
namespace BenchmarkDotNet.IntegrationTests.Diagnosers
1413
{
@@ -26,10 +25,7 @@ public sealed class MockInProcessDiagnoser : IInProcessDiagnoser
2625

2726
public void DisplayResults(ILogger logger) => logger.WriteLine($"{nameof(MockInProcessDiagnoser)} results: [{string.Join(", ", Results.Values)}]");
2827

29-
public IInProcessDiagnoserHandler GetHandler(BenchmarkCase benchmarkCase, int index) => new MockInProcessDiagnoserHandler(index, GetRunMode(benchmarkCase));
30-
31-
public string GetHandlerSourceCode(BenchmarkCase benchmarkCase, int index)
32-
=> $"new {typeof(MockInProcessDiagnoserHandler).GetCorrectCSharpTypeName()}({index}, {SourceCodeHelper.ToSourceCode(GetRunMode(benchmarkCase))})";
28+
public IInProcessDiagnoserHandler? GetHandler(BenchmarkCase benchmarkCase) => new MockInProcessDiagnoserHandler();
3329

3430
public RunMode GetRunMode(BenchmarkCase benchmarkCase) => RunMode.NoOverhead;
3531

@@ -40,14 +36,13 @@ public void Handle(HostSignal signal, DiagnoserActionParameters parameters) { }
4036
public IEnumerable<ValidationError> Validate(ValidationParameters validationParameters) => [];
4137
}
4238

43-
public sealed class MockInProcessDiagnoserHandler(int index, RunMode runMode) : IInProcessDiagnoserHandler
39+
public sealed class MockInProcessDiagnoserHandler : IInProcessDiagnoserHandler
4440
{
45-
public int Index { get; } = index;
46-
47-
public RunMode RunMode { get; } = runMode;
48-
4941
public void Handle(BenchmarkSignal signal, InProcessDiagnoserActionArgs parameters) { }
5042

51-
public string SerializeResults() => $"DummyResult{Index}";
43+
public string SerializeResults() => "MockResult";
44+
45+
public string ToSourceCode()
46+
=> $"new {typeof(MockInProcessDiagnoserHandler).GetCorrectCSharpTypeName()}()";
5247
}
5348
}

‎tests/BenchmarkDotNet.IntegrationTests/InProcessEmitTest.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ public void InProcessEmitSupportsInProcessDiagnosers()
140140

141141
var summary = CanExecute<BenchmarkAllCases>(config);
142142

143-
var expected = Enumerable.Repeat("DummyResult0", summary.BenchmarksCases.Length);
143+
var expected = Enumerable.Repeat("MockResult", summary.BenchmarksCases.Length);
144144
Assert.Equal(expected, diagnoser.Results.Values);
145145
}
146146

0 commit comments

Comments
 (0)