Skip to content
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

Improve Disassembly exporters and add PrettyGithubMarkdownDiffDisassemblyExporter #927

Merged
merged 5 commits into from
Oct 30, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/BenchmarkDotNet/Diagnosers/DisassemblyDiagnoser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ private DisassemblyDiagnoser(WindowsDisassembler windowsDisassembler, MonoDisass
new CombinedDisassemblyExporter(results),
new RawDisassemblyExporter(results),
new PrettyHtmlDisassemblyExporter(results),
new PrettyGithubMarkdownDisassemblyExporter(results)
new PrettyGithubMarkdownDisassemblyExporter(results),
new PrettyGithubMarkdownDiffDisassemblyExporter(results)
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Loggers;
using BenchmarkDotNet.Reports;
using BenchmarkDotNet.Running;
using StreamWriter = BenchmarkDotNet.Portability.StreamWriter;

namespace BenchmarkDotNet.Exporters
{
public class PrettyGithubMarkdownDiffDisassemblyExporter : PrettyGithubMarkdownDisassemblyExporterBase
{
private readonly IReadOnlyDictionary<BenchmarkCase, DisassemblyResult> results;

private static bool canRead;

protected override string FileExtension => "md";
protected override string FileCaption => "asm.pretty.diff";

public PrettyGithubMarkdownDiffDisassemblyExporter(IReadOnlyDictionary<BenchmarkCase, DisassemblyResult> results)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: =>

this.results = results;
}

public override void ExportToLog(Summary summary, ILogger logger)
{
var benchmarksCases = summary.BenchmarksCases
.Where(results.ContainsKey).ToList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if you don't intend to add any elements to a materialized collection, you should prefer array over list. The best choice would be ImmutableArray - materialized, fast and immutable


logger.WriteLine($"## {summary.Title}");

for (int i = 0; i < benchmarksCases.Count; i++)
{
var firstBenchmarkCase = benchmarksCases[i];
for (int j = i + 1; j < benchmarksCases.Count; j++)
{
var secondBenchmarkCase = benchmarksCases[j];

var firstFileName = Export(summary, results[firstBenchmarkCase]);
var secondFileName = Export(summary, results[secondBenchmarkCase]);
try
{
var builder = new StringBuilder();

RunGitDiff(firstFileName, secondFileName, builder);

if (firstBenchmarkCase.Descriptor.WorkloadMethod == secondBenchmarkCase.Descriptor.WorkloadMethod) // diff between the same method for different JITs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is the only case when we should be producing the diff if we want to have it enabled by default.

Example: a ListBenchmarks type with 3 benchmarks: Add, AddRange and Insert. When we use the disassembly diagnoser we want to get the disassembly for all 3 of them. But we are rather not interested in diff.
When we run it for few different Runtime settings, then we want to get the diff.

{
logger.WriteLine($"**Diff for {firstBenchmarkCase.Descriptor.WorkloadMethod.Name} method between:**");
logger.WriteLine($"{GetImportantInfo(summary[firstBenchmarkCase])}");
logger.WriteLine($"{GetImportantInfo(summary[secondBenchmarkCase])}");
}
else // different methods, same JIT
{
logger.WriteLine(
$"**Diff between {firstBenchmarkCase.Descriptor.WorkloadMethod.Name} and {secondBenchmarkCase.Descriptor.WorkloadMethod.Name}**");
logger.WriteLine($"on {GetImportantInfo(summary[firstBenchmarkCase])}.");
}

logger.WriteLine("```diff");
logger.WriteLine(builder.ToString());
logger.WriteLine("```");
}
finally
{
File.Delete(firstFileName);
File.Delete(secondFileName);
}
}
}
}

private static string Export(Summary summary, DisassemblyResult disassemblyResult)
{
string filePath = $"{Path.Combine(summary.ResultsDirectoryPath, Guid.NewGuid().ToString())}-diff.temp";

if (File.Exists(filePath))
File.Delete(filePath);

using (var stream = StreamWriter.FromPath(filePath))
{
Export(new StreamLogger(stream), disassemblyResult, quotingCode: false);
}

return filePath;
}

private static string GetImportantInfo(BenchmarkReport benchmarkReport) => benchmarkReport.GetRuntimeInfo();

private static void RunGitDiff(string firstFile, string secondFile, StringBuilder result)
{
var startInfo = new ProcessStartInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to reuse ProcessHelper.RunAndReadOutput here and simplify the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reuse ProcessHelper.RunAndReadOutputLineByLine because ProcessHelper.RunAndReadOutput doesn't throw exception in case git is not installed on the system. There is a problem:

try
{
process.Start();
}
catch (Exception)
{
return null;
}

I can refactor ProcessHelper but I think it should be a different task.

{
FileName = "git",
Arguments = $"diff --no-index --no-color --text {firstFile} {secondFile}",
UseShellExecute = false,
RedirectStandardOutput = true,
};
canRead = false;

try
{
using (var testProcess = Process.Start(startInfo))
{
testProcess.OutputDataReceived += (s, e) => ProcessOutput(e.Data, result);
testProcess.BeginOutputReadLine();

testProcess.WaitForExit();
}
}
catch (Exception ex)
{
result.AppendLine("An exception occurred during run Git. Please check if you have Git installed on your system and Git is added to PATH.");
result.AppendLine($"Exception: {ex.Message}");
}
}

private static void ProcessOutput(string line, StringBuilder result)
{
if (!string.IsNullOrEmpty(line) && line.Contains("@@"))
{
canRead = true;
return;
}

if (canRead)
{
result.AppendLine(line);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,84 +1,36 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Loggers;
using BenchmarkDotNet.Reports;
using BenchmarkDotNet.Running;
using StreamWriter = BenchmarkDotNet.Portability.StreamWriter;

namespace BenchmarkDotNet.Exporters
{
public class PrettyGithubMarkdownDisassemblyExporter : IExporter
public class PrettyGithubMarkdownDisassemblyExporter : PrettyGithubMarkdownDisassemblyExporterBase
{
private readonly IReadOnlyDictionary<BenchmarkCase, DisassemblyResult> results;

public PrettyGithubMarkdownDisassemblyExporter(IReadOnlyDictionary<BenchmarkCase, DisassemblyResult> results) => this.results = results;

public string Name => nameof(PrettyGithubMarkdownDisassemblyExporter);

public void ExportToLog(Summary summary, ILogger logger) { }

public IEnumerable<string> ExportToFiles(Summary summary, ILogger consoleLogger)
=> summary.BenchmarksCases
.Where(results.ContainsKey)
.Select(benchmark => Export(summary, benchmark));

private string Export(Summary summary, BenchmarkCase benchmarkCase)
public PrettyGithubMarkdownDisassemblyExporter(IReadOnlyDictionary<BenchmarkCase, DisassemblyResult> results)
{
string filePath = $"{Path.Combine(summary.ResultsDirectoryPath, benchmarkCase.FolderInfo)}-asm.pretty.md";
if (File.Exists(filePath))
File.Delete(filePath);

using (var stream = StreamWriter.FromPath(filePath))
{
Export(new StreamLogger(stream), results[benchmarkCase], benchmarkCase);
}

return filePath;
this.results = results;
}

private static void Export(ILogger logger, DisassemblyResult disassemblyResult, BenchmarkCase benchmarkCase)
{
int methodIndex = 0;
foreach (var method in disassemblyResult.Methods.Where(method => string.IsNullOrEmpty(method.Problem)))
{
logger.WriteLine("```assembly");
logger.WriteLine($"; {method.Name}");

var pretty = DisassemblyPrettifier.Prettify(method, $"M{methodIndex++:00}");

uint totalSizeInBytes = 0;
foreach (var element in pretty)
{
if (element is DisassemblyPrettifier.Label label)
{
logger.WriteLine($"{label.TextRepresentation}:");

continue;
}
if (element.Source is Asm asm)
{
totalSizeInBytes += asm.SizeInBytes;
}

logger.WriteLine($" {element.TextRepresentation}");
}
protected override string FileExtension => "md";
protected override string FileCaption => "asm.pretty";

logger.WriteLine($"; Total bytes of code {totalSizeInBytes}");
logger.WriteLine("```");
}
public override void ExportToLog(Summary summary, ILogger logger)
{
var benchmarksCases = summary.BenchmarksCases
.Where(results.ContainsKey);

foreach (var withProblems in disassemblyResult.Methods
.Where(method => !string.IsNullOrEmpty(method.Problem))
.GroupBy(method => method.Problem))
foreach (var benchmarkCase in benchmarksCases)
{
logger.WriteLine($"**{withProblems.Key}**");
foreach (var withProblem in withProblems)
{
logger.WriteLine(withProblem.Name);
}
logger.WriteLine($"## {GetImportantInfo(summary[benchmarkCase])}");
Export(logger, results[benchmarkCase]);
}
}

private static string GetImportantInfo(BenchmarkReport benchmarkReport) => benchmarkReport.GetRuntimeInfo();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using System.Linq;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Loggers;

namespace BenchmarkDotNet.Exporters
{
public abstract class PrettyGithubMarkdownDisassemblyExporterBase : ExporterBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guilty of creating too many classes and too deep inheritance hierarchy in BenchmarkDotNet. We should not be doing this anymore (Composition over inheritance).

Could you put this method back to PrettyGithubMarkdownDisassemblyExporter and do the following;

  1. make it static internal
  2. make PrettyGithubMarkdownDisassemblyExporter and PrettyGithubMarkdownDiffDisassemblyExporter inherit directly from ExporterBase
  3. reuse PrettyGithubMarkdownDisassemblyExporter.Export in PrettyGithubMarkdownDiffDisassemblyExporter

{
protected static void Export(ILogger logger, DisassemblyResult disassemblyResult, bool quotingCode = true)
{
int methodIndex = 0;
foreach (var method in disassemblyResult.Methods.Where(method => string.IsNullOrEmpty(method.Problem)))
{
if (quotingCode)
{
logger.WriteLine("```assembly");
}

logger.WriteLine($"; {method.Name}");

var pretty = DisassemblyPrettifier.Prettify(method, $"M{methodIndex++:00}");

uint totalSizeInBytes = 0;
foreach (var element in pretty)
{
if (element is DisassemblyPrettifier.Label label)
{
logger.WriteLine($"{label.TextRepresentation}:");

continue;
}
if (element.Source is Asm asm)
{
totalSizeInBytes += asm.SizeInBytes;
}

logger.WriteLine($" {element.TextRepresentation}");
}

logger.WriteLine($"; Total bytes of code {totalSizeInBytes}");
if (quotingCode)
{
logger.WriteLine("```");
}
}

foreach (var withProblems in disassemblyResult.Methods
.Where(method => !string.IsNullOrEmpty(method.Problem))
.GroupBy(method => method.Problem))
{
logger.WriteLine($"**{withProblems.Key}**");
foreach (var withProblem in withProblems)
{
logger.WriteLine(withProblem.Name);
}
}

logger.WriteLine();
}
}
}
Loading