From 993511b12f555319763020ca39a4548e163a2560 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Oct 2021 15:40:15 +0200 Subject: [PATCH 1/4] Support passing total code size through when doing SPMI diffs Currently the total code bytes shown for the base/diff is misleading when SPMI is used because SPMI generates only .dasm files for method contexts with diffs. This change: * Adds -baseMetricsSummary and -diffMetricsSummary to SPMI, which specifies a file path to output metrics. Currently that's just the number of failing/succeeding compiles, and the total number of code bytes, but the hope is to use this for other metrics as well. * Adds --override-total-base-metric and --override-total-diff-metric to jit-analyze, to support overriding the computed total base and total diff metric, since they are misleading when .dasm files are only created for differences * Supports this from the superpmi.py wrapper script for asmdiffs when no explicit metric is provided: in this case, the script will get the metrics from SPMI and pass them to jit-analyze to override the computed totals. The net result is that the displayed results from jit-analyze after SPMI runs are less misleading and should be more comparable to similar PMI runs. --- .../ToolBox/superpmi/superpmi/CMakeLists.txt | 1 + .../ToolBox/superpmi/superpmi/commandline.cpp | 30 +++ .../ToolBox/superpmi/superpmi/commandline.h | 4 + .../ToolBox/superpmi/superpmi/jitinstance.cpp | 10 +- .../ToolBox/superpmi/superpmi/jitinstance.h | 2 +- .../superpmi/superpmi/methodstatsemitter.h | 1 + .../superpmi/superpmi/metricssummary.cpp | 48 +++++ .../superpmi/superpmi/metricssummary.h | 26 +++ .../superpmi/superpmi/parallelsuperpmi.cpp | 189 +++++++++++++----- .../ToolBox/superpmi/superpmi/superpmi.cpp | 18 +- src/coreclr/scripts/superpmi.py | 48 ++++- 11 files changed, 322 insertions(+), 55 deletions(-) create mode 100644 src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp create mode 100644 src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h diff --git a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt index 92cda6d5600feb..4827c195dc2997 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt +++ b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt @@ -22,6 +22,7 @@ set(SUPERPMI_SOURCES jitdebugger.cpp jitinstance.cpp methodstatsemitter.cpp + metricssummary.cpp neardiffer.cpp parallelsuperpmi.cpp superpmi.cpp diff --git a/src/coreclr/ToolBox/superpmi/superpmi/commandline.cpp b/src/coreclr/ToolBox/superpmi/superpmi/commandline.cpp index e4d98a13752d60..c4735b23d7527b 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/commandline.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/commandline.cpp @@ -81,6 +81,16 @@ void CommandLine::DumpHelp(const char* program) printf(" t - method throughput time\n"); printf(" * - all available method stats\n"); printf("\n"); + printf(" -metricsSummary , -baseMetricsSummary \n"); + printf(" Emit a summary of metrics to the specified file\n"); + printf(" Currently includes:\n"); + printf(" Total number of successful SPMI compiles\n"); + printf(" Total number of failing SPMI compiles\n"); + printf(" Total amount of ASM code in bytes\n"); + printf("\n"); + printf(" -diffMetricsSummary \n"); + printf(" Same as above, but emit for the diff/second JIT"); + printf("\n"); printf(" -a[pplyDiff]\n"); printf(" Compare the compile result generated from the provided JIT with the\n"); printf(" compile result stored with the MC. If two JITs are provided, this\n"); @@ -374,6 +384,26 @@ bool CommandLine::Parse(int argc, char* argv[], /* OUT */ Options* o) o->methodStatsTypes = argv[i]; } + else if ((_strnicmp(&argv[i][1], "metricsSummary", argLen) == 0) || (_strnicmp(&argv[i][1], "baseMetricsSummary", argLen) == 0)) + { + if (++i >= argc) + { + DumpHelp(argv[0]); + return false; + } + + o->baseMetricsSummaryFile = argv[i]; + } + else if ((_strnicmp(&argv[i][1], "diffMetricsSummary", argLen) == 0)) + { + if (++i >= argc) + { + DumpHelp(argv[0]); + return false; + } + + o->diffMetricsSummaryFile = argv[i]; + } else if ((_strnicmp(&argv[i][1], "applyDiff", argLen) == 0)) { o->applyDiff = true; diff --git a/src/coreclr/ToolBox/superpmi/superpmi/commandline.h b/src/coreclr/ToolBox/superpmi/superpmi/commandline.h index e9bae5740262b4..055adf00295cda 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/commandline.h +++ b/src/coreclr/ToolBox/superpmi/superpmi/commandline.h @@ -36,6 +36,8 @@ class CommandLine , indexes(nullptr) , hash(nullptr) , methodStatsTypes(nullptr) + , baseMetricsSummaryFile(nullptr) + , diffMetricsSummaryFile(nullptr) , mclFilename(nullptr) , diffMCLFilename(nullptr) , targetArchitecture(nullptr) @@ -67,6 +69,8 @@ class CommandLine int* indexes; char* hash; char* methodStatsTypes; + char* baseMetricsSummaryFile; + char* diffMetricsSummaryFile; char* mclFilename; char* diffMCLFilename; char* targetArchitecture; diff --git a/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp b/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp index fafc367669d47c..96484d4b86bcd1 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp @@ -8,6 +8,7 @@ #include "jithost.h" #include "errorhandling.h" #include "spmiutil.h" +#include "metricssummary.h" JitInstance* JitInstance::InitJit(char* nameOfJit, bool breakOnAssert, @@ -276,7 +277,7 @@ bool JitInstance::reLoad(MethodContext* firstContext) return true; } -JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput) +JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, MetricsSummary* metrics) { struct Param : FilterSuperPMIExceptionsParam_CaptureException { @@ -286,12 +287,14 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i unsigned flags; int mcIndex; bool collectThroughput; + MetricsSummary* metrics; } param; param.pThis = this; param.result = RESULT_SUCCESS; // assume success param.flags = 0; param.mcIndex = mcIndex; param.collectThroughput = collectThroughput; + param.metrics = metrics; // store to instance field our raw values, so we can figure things out a bit later... mc = MethodToCompile; @@ -365,11 +368,16 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i pParam->pThis->mc->cr->recAllocGCInfoCapture(); pParam->pThis->mc->cr->recMessageLog("Successful Compile"); + + pParam->metrics->SuccessfulCompiles++; + pParam->metrics->NumCodeBytes += NCodeSizeBlock; } else { LogDebug("compileMethod failed with result %d", jitResult); pParam->result = RESULT_ERROR; + + pParam->metrics->FailingCompiles++; } } PAL_EXCEPT_FILTER(FilterSuperPMIExceptions_CaptureExceptionAndStop) diff --git a/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.h b/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.h index c572008481a65d..8e8fbabf82ead4 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.h +++ b/src/coreclr/ToolBox/superpmi/superpmi/jitinstance.h @@ -60,7 +60,7 @@ class JitInstance bool resetConfig(MethodContext* firstContext); - Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput); + Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, class MetricsSummary* summary); const WCHAR* getForceOption(const WCHAR* key); const WCHAR* getOption(const WCHAR* key); diff --git a/src/coreclr/ToolBox/superpmi/superpmi/methodstatsemitter.h b/src/coreclr/ToolBox/superpmi/superpmi/methodstatsemitter.h index 083d24fe852a94..40d38698b3d446 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/methodstatsemitter.h +++ b/src/coreclr/ToolBox/superpmi/superpmi/methodstatsemitter.h @@ -24,4 +24,5 @@ class MethodStatsEmitter void Emit(int methodNumber, MethodContext* mc, ULONGLONG firstTime, ULONGLONG secondTime); void SetStatsTypes(char* types); }; + #endif diff --git a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp new file mode 100644 index 00000000000000..0b5a620b5b0d5c --- /dev/null +++ b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#include "standardpch.h" +#include "metricssummary.h" +#include "logging.h" + +#include +#include + +bool MetricsSummary::SaveToFile(const char* path) +{ + FILE* file = fopen(path, "w"); + if (file == nullptr) + { + return false; + } + + fprintf(file, "Successful compiles,Failing compiles,Code bytes\n"); + fprintf(file, "%d,%d,%" PRId64 "\n", SuccessfulCompiles, FailingCompiles, NumCodeBytes); + fclose(file); + return true; +} + +bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* result) +{ + FILE* file = fopen(path, "r"); + if (file == nullptr) + { + return false; + } + + fscanf(file, "Successful compiles,Failing compiles,Code bytes\n"); + if (fscanf(file, "%d,%d,%" SCNd64 "\n", &result->SuccessfulCompiles, &result->FailingCompiles, &result->NumCodeBytes) != 3) + { + return false; + } + + fclose(file); + return true; +} + +void MetricsSummary::AggregateFrom(const MetricsSummary& other) +{ + SuccessfulCompiles += other.SuccessfulCompiles; + FailingCompiles += other.FailingCompiles; + NumCodeBytes += other.NumCodeBytes; +} diff --git a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h new file mode 100644 index 00000000000000..6ed0922152ec65 --- /dev/null +++ b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef _MetricsSummary +#define _MetricsSummary + +class MetricsSummary +{ +public: + int SuccessfulCompiles; + int FailingCompiles; + int64_t NumCodeBytes; + + MetricsSummary() + : SuccessfulCompiles(0) + , FailingCompiles(0) + , NumCodeBytes(0) + { + } + + bool SaveToFile(const char* path); + static bool LoadFromFile(const char* path, MetricsSummary* result); + void AggregateFrom(const MetricsSummary& other); +}; + +#endif diff --git a/src/coreclr/ToolBox/superpmi/superpmi/parallelsuperpmi.cpp b/src/coreclr/ToolBox/superpmi/superpmi/parallelsuperpmi.cpp index 46c1a7a91132d0..92b172e196d1c0 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/parallelsuperpmi.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/parallelsuperpmi.cpp @@ -8,6 +8,7 @@ #include "lightweightmap.h" #include "commandline.h" #include "errorhandling.h" +#include "metricssummary.h" #define MAX_LOG_LINE_SIZE 0x1000 // 4 KB @@ -293,6 +294,35 @@ void ProcessChildStdOut(const CommandLine::Options& o, } } +static bool ProcessChildMetrics(const char* baseMetricsSummaryPath, const char* diffMetricsSummaryPath, MetricsSummary* baseMetrics, MetricsSummary* diffMetrics) +{ + if (baseMetricsSummaryPath != nullptr) + { + MetricsSummary childBaseMetrics; + if (!MetricsSummary::LoadFromFile(baseMetricsSummaryPath, &childBaseMetrics)) + { + LogError("Couldn't load base metrics summary created by child process"); + return false; + } + + baseMetrics->AggregateFrom(childBaseMetrics); + } + + if (diffMetricsSummaryPath != nullptr) + { + MetricsSummary childDiffMetrics; + if (!MetricsSummary::LoadFromFile(diffMetricsSummaryPath, &childDiffMetrics)) + { + LogError("Couldn't load diff metrics summary created by child process"); + return false; + } + + diffMetrics->AggregateFrom(childDiffMetrics); + } + + return true; +} + #ifndef TARGET_UNIX // TODO-Porting: handle Ctrl-C signals gracefully on Unix BOOL WINAPI CtrlHandler(DWORD fdwCtrlType) { @@ -309,15 +339,39 @@ int __cdecl compareInt(const void* arg1, const void* arg2) return (*(const int*)arg1) - (*(const int*)arg2); } -// 'arrWorkerMCLPath' is an array of strings of size 'workerCount'. -void MergeWorkerMCLs(char* mclFilename, char** arrWorkerMCLPath, int workerCount) +struct PerWorkerData +{ + HANDLE hStdOutput; + HANDLE hStdError; + + char* failingMCListPath; + char* diffMCListPath; + char* stdOutputPath; + char* stdErrorPath; + char* baseMetricsSummaryPath; + char* diffMetricsSummaryPath; + + PerWorkerData() + : hStdOutput(INVALID_HANDLE_VALUE) + , hStdError(INVALID_HANDLE_VALUE) + , failingMCListPath(nullptr) + , diffMCListPath(nullptr) + , stdOutputPath(nullptr) + , stdErrorPath(nullptr) + , baseMetricsSummaryPath(nullptr) + , diffMetricsSummaryPath(nullptr) + { + } +}; + +void MergeWorkerMCLs(char* mclFilename, PerWorkerData* workerData, int workerCount, char* PerWorkerData::*mclPath) { int **MCL = new int *[workerCount], *MCLCount = new int[workerCount], totalCount = 0; for (int i = 0; i < workerCount; i++) { // Read the next partial MCL file - ReadMCLToArray(arrWorkerMCLPath[i], &MCL[i], &MCLCount[i]); + ReadMCLToArray(workerData[i].*mclPath, &MCL[i], &MCLCount[i]); totalCount += MCLCount[i]; } @@ -474,14 +528,7 @@ int doParallelSuperPMI(CommandLine::Options& o) LogVerbose(" diffMCLFilename=%s", o.diffMCLFilename); LogVerbose(" workerCount=%d, skipCleanup=%d.", o.workerCount, o.skipCleanup); - HANDLE* hProcesses = new HANDLE[o.workerCount]; - HANDLE* hStdOutput = new HANDLE[o.workerCount]; - HANDLE* hStdError = new HANDLE[o.workerCount]; - - char** arrFailingMCListPath = new char*[o.workerCount]; - char** arrDiffMCListPath = new char*[o.workerCount]; - char** arrStdOutputPath = new char*[o.workerCount]; - char** arrStdErrorPath = new char*[o.workerCount]; + PerWorkerData* perWorkerData = new PerWorkerData[o.workerCount]; // Add a random number to the temporary file names to allow multiple parallel SuperPMI to happen at once. unsigned int randNumber = 0; @@ -493,51 +540,71 @@ int doParallelSuperPMI(CommandLine::Options& o) for (int i = 0; i < o.workerCount; i++) { + PerWorkerData& wd = perWorkerData[i]; if (o.mclFilename != nullptr) { - arrFailingMCListPath[i] = new char[MAX_PATH]; - sprintf_s(arrFailingMCListPath[i], MAX_PATH, "%sParallelSuperPMI-%u-%d.mcl", tempPath, randNumber, i); + wd.failingMCListPath = new char[MAX_PATH]; + sprintf_s(wd.failingMCListPath, MAX_PATH, "%sParallelSuperPMI-%u-%d.mcl", tempPath, randNumber, i); } - else + + if (o.diffMCLFilename != nullptr) { - arrFailingMCListPath[i] = nullptr; + wd.diffMCListPath = new char[MAX_PATH]; + sprintf_s(wd.diffMCListPath, MAX_PATH, "%sParallelSuperPMI-Diff-%u-%d.mcl", tempPath, randNumber, i); } - if (o.diffMCLFilename != nullptr) + if (o.baseMetricsSummaryFile != nullptr) { - arrDiffMCListPath[i] = new char[MAX_PATH]; - sprintf_s(arrDiffMCListPath[i], MAX_PATH, "%sParallelSuperPMI-Diff-%u-%d.mcl", tempPath, randNumber, i); + wd.baseMetricsSummaryPath = new char[MAX_PATH]; + sprintf_s(wd.baseMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-BaseMetricsSummary-%u-%d.txt", tempPath, randNumber, i); } - else + + if (o.diffMetricsSummaryFile != nullptr) { - arrDiffMCListPath[i] = nullptr; + wd.diffMetricsSummaryPath = new char[MAX_PATH]; + sprintf_s(wd.diffMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-DiffMetricsSummary-%u-%d.txt", tempPath, randNumber, i); } - arrStdOutputPath[i] = new char[MAX_PATH]; - arrStdErrorPath[i] = new char[MAX_PATH]; + wd.stdOutputPath = new char[MAX_PATH]; + wd.stdErrorPath = new char[MAX_PATH]; - sprintf_s(arrStdOutputPath[i], MAX_PATH, "%sParallelSuperPMI-stdout-%u-%d.txt", tempPath, randNumber, i); - sprintf_s(arrStdErrorPath[i], MAX_PATH, "%sParallelSuperPMI-stderr-%u-%d.txt", tempPath, randNumber, i); + sprintf_s(wd.stdOutputPath, MAX_PATH, "%sParallelSuperPMI-stdout-%u-%d.txt", tempPath, randNumber, i); + sprintf_s(wd.stdErrorPath, MAX_PATH, "%sParallelSuperPMI-stderr-%u-%d.txt", tempPath, randNumber, i); } char cmdLine[MAX_CMDLINE_SIZE]; cmdLine[0] = '\0'; int bytesWritten; + HANDLE* hProcesses = new HANDLE[o.workerCount]; for (int i = 0; i < o.workerCount; i++) { bytesWritten = sprintf_s(cmdLine, MAX_CMDLINE_SIZE, "%s -stride %d %d", spmiFilename, i + 1, o.workerCount); - if (o.mclFilename != nullptr) + PerWorkerData& wd = perWorkerData[i]; + + if (wd.failingMCListPath != nullptr) { bytesWritten += sprintf_s(cmdLine + bytesWritten, MAX_CMDLINE_SIZE - bytesWritten, " -failingMCList %s", - arrFailingMCListPath[i]); + wd.failingMCListPath); } - if (o.diffMCLFilename != nullptr) + if (wd.diffMCListPath != nullptr) { bytesWritten += sprintf_s(cmdLine + bytesWritten, MAX_CMDLINE_SIZE - bytesWritten, " -diffMCList %s", - arrDiffMCListPath[i]); + wd.diffMCListPath); + } + + if (wd.baseMetricsSummaryPath != nullptr) + { + bytesWritten += sprintf_s(cmdLine + bytesWritten, MAX_CMDLINE_SIZE - bytesWritten, " -baseMetricsSummary %s", + wd.baseMetricsSummaryPath); + } + + if (wd.diffMetricsSummaryPath != nullptr) + { + bytesWritten += sprintf_s(cmdLine + bytesWritten, MAX_CMDLINE_SIZE - bytesWritten, " -diffMetricsSummary %s", + wd.diffMetricsSummaryPath); } if (o.failureLimit > 0) @@ -553,26 +620,26 @@ int doParallelSuperPMI(CommandLine::Options& o) sa.lpSecurityDescriptor = NULL; sa.bInheritHandle = TRUE; // Let newly created stdout/stderr handles be inherited. - LogDebug("stdout %i=%s", i, arrStdOutputPath[i]); - hStdOutput[i] = CreateFileA(arrStdOutputPath[i], GENERIC_WRITE, FILE_SHARE_READ, &sa, CREATE_ALWAYS, + LogDebug("stdout %i=%s", i, wd.stdOutputPath); + wd.hStdOutput = CreateFileA(wd.stdOutputPath, GENERIC_WRITE, FILE_SHARE_READ, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); - if (hStdOutput[i] == INVALID_HANDLE_VALUE) + if (wd.hStdOutput == INVALID_HANDLE_VALUE) { - LogError("Unable to open '%s'. GetLastError()=%u", arrStdOutputPath[i], GetLastError()); + LogError("Unable to open '%s'. GetLastError()=%u", wd.stdOutputPath, GetLastError()); return -1; } - LogDebug("stderr %i=%s", i, arrStdErrorPath[i]); - hStdError[i] = CreateFileA(arrStdErrorPath[i], GENERIC_WRITE, FILE_SHARE_READ, &sa, CREATE_ALWAYS, + LogDebug("stderr %i=%s", i, wd.stdErrorPath); + wd.hStdError = CreateFileA(wd.stdErrorPath, GENERIC_WRITE, FILE_SHARE_READ, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); - if (hStdError[i] == INVALID_HANDLE_VALUE) + if (wd.hStdError == INVALID_HANDLE_VALUE) { - LogError("Unable to open '%s'. GetLastError()=%u", arrStdErrorPath[i], GetLastError()); + LogError("Unable to open '%s'. GetLastError()=%u", wd.stdErrorPath, GetLastError()); return -1; } // Create a SuperPMI worker process and redirect its output to file - if (!StartProcess(cmdLine, hStdOutput[i], hStdError[i], &hProcesses[i])) + if (!StartProcess(cmdLine, wd.hStdOutput, wd.hStdError, &hProcesses[i])) { return -1; } @@ -583,8 +650,8 @@ int doParallelSuperPMI(CommandLine::Options& o) // Close stdout/stderr for (int i = 0; i < o.workerCount; i++) { - CloseHandle(hStdOutput[i]); - CloseHandle(hStdError[i]); + CloseHandle(perWorkerData[i].hStdOutput); + CloseHandle(perWorkerData[i].hStdError); } SpmiResult result = SpmiResult::Success; @@ -626,13 +693,18 @@ int doParallelSuperPMI(CommandLine::Options& o) bool usageError = false; // variable to flag if we hit a usage error in SuperPMI int loaded = 0, jitted = 0, failed = 0, excluded = 0, missing = 0, diffs = 0; + MetricsSummary baseMetrics; + MetricsSummary diffMetrics; // Read the stderr files and log them as errors // Read the stdout files and parse them for counts and log any MISSING or ISSUE errors for (int i = 0; i < o.workerCount; i++) { - ProcessChildStdErr(arrStdErrorPath[i]); - ProcessChildStdOut(o, arrStdOutputPath[i], &loaded, &jitted, &failed, &excluded, &missing, &diffs, &usageError); + PerWorkerData& wd = perWorkerData[i]; + ProcessChildStdErr(wd.stdErrorPath); + ProcessChildStdOut(o, wd.stdOutputPath, &loaded, &jitted, &failed, &excluded, &missing, &diffs, &usageError); + ProcessChildMetrics(wd.baseMetricsSummaryPath, wd.diffMetricsSummaryPath, &baseMetrics, &diffMetrics); + if (usageError) break; } @@ -640,13 +712,23 @@ int doParallelSuperPMI(CommandLine::Options& o) if (o.mclFilename != nullptr && !usageError) { // Concat the resulting .mcl files - MergeWorkerMCLs(o.mclFilename, arrFailingMCListPath, o.workerCount); + MergeWorkerMCLs(o.mclFilename, perWorkerData, o.workerCount, &PerWorkerData::failingMCListPath); } if (o.diffMCLFilename != nullptr && !usageError) { // Concat the resulting diff .mcl files - MergeWorkerMCLs(o.diffMCLFilename, arrDiffMCListPath, o.workerCount); + MergeWorkerMCLs(o.diffMCLFilename, perWorkerData, o.workerCount, &PerWorkerData::diffMCListPath); + } + + if (o.baseMetricsSummaryFile != nullptr && !usageError) + { + baseMetrics.SaveToFile(o.baseMetricsSummaryFile); + } + + if (o.diffMetricsSummaryFile != nullptr && !usageError) + { + diffMetrics.SaveToFile(o.diffMetricsSummaryFile); } if (!usageError) @@ -670,16 +752,25 @@ int doParallelSuperPMI(CommandLine::Options& o) // Delete all temporary files generated for (int i = 0; i < o.workerCount; i++) { - if (arrFailingMCListPath[i] != nullptr) + PerWorkerData& wd = perWorkerData[i]; + if (wd.failingMCListPath != nullptr) + { + DeleteFile(wd.failingMCListPath); + } + if (wd.diffMCListPath != nullptr) + { + DeleteFile(wd.diffMCListPath); + } + if (wd.baseMetricsSummaryPath != nullptr) { - DeleteFile(arrFailingMCListPath[i]); + DeleteFile(wd.baseMetricsSummaryPath); } - if (arrDiffMCListPath[i] != nullptr) + if (wd.diffMetricsSummaryPath != nullptr) { - DeleteFile(arrDiffMCListPath[i]); + DeleteFile(wd.diffMetricsSummaryPath); } - DeleteFile(arrStdOutputPath[i]); - DeleteFile(arrStdErrorPath[i]); + DeleteFile(wd.stdOutputPath); + DeleteFile(wd.stdErrorPath); } } diff --git a/src/coreclr/ToolBox/superpmi/superpmi/superpmi.cpp b/src/coreclr/ToolBox/superpmi/superpmi/superpmi.cpp index 640dfd367a7f36..5410240bdad331 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/superpmi.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/superpmi.cpp @@ -18,6 +18,7 @@ #include "mclist.h" #include "methodstatsemitter.h" #include "spmiutil.h" +#include "metricssummary.h" extern int doParallelSuperPMI(CommandLine::Options& o); @@ -264,6 +265,9 @@ int __cdecl main(int argc, char* argv[]) } } + MetricsSummary baseMetrics; + MetricsSummary diffMetrics; + while (true) { MethodContextBuffer mcb = reader->GetNextMethodContext(); @@ -360,7 +364,7 @@ int __cdecl main(int argc, char* argv[]) jittedCount++; st3.Start(); - res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput); + res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &baseMetrics); st3.Stop(); LogDebug("Method %d compiled in %fms, result %d", reader->GetMethodContextIndex(), st3.GetMilliseconds(), res); @@ -378,7 +382,7 @@ int __cdecl main(int argc, char* argv[]) mc->cr = new CompileResult(); st4.Start(); - res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput); + res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &diffMetrics); st4.Stop(); LogDebug("Method %d compiled by JIT2 in %fms, result %d", reader->GetMethodContextIndex(), st4.GetMilliseconds(), res2); @@ -603,6 +607,16 @@ int __cdecl main(int argc, char* argv[]) st2.Stop(); LogVerbose("Total time: %fms", st2.GetMilliseconds()); + if (o.baseMetricsSummaryFile != nullptr) + { + baseMetrics.SaveToFile(o.baseMetricsSummaryFile); + } + + if (o.diffMetricsSummaryFile != nullptr) + { + diffMetrics.SaveToFile(o.diffMetricsSummaryFile); + } + if (methodStatsEmitter != nullptr) { delete methodStatsEmitter; diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 4b502f0bffa4f5..7618c11480ec0e 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -18,6 +18,7 @@ import argparse import asyncio +import csv import datetime import locale import logging @@ -800,6 +801,22 @@ def is_url(path): # If it doesn't look like an URL, treat it like a file, possibly a UNC file. return path.lower().startswith("http:") or path.lower().startswith("https:") +def read_csv_metrics(path): + """ Read a metrics summary file produced by superpmi, and return the single row containing the information as a dictionary. + + Args: + path (str) : path to .csv file + + Returns: + A dictionary with each metric + """ + + with open(path) as csv_file: + reader = csv.DictReader(csv_file) + for row in reader: + return row + + return None ################################################################################ # Helper classes @@ -1800,6 +1817,9 @@ def replay_with_asm_diffs(self): fail_mcl_file = os.path.join(temp_location, os.path.basename(mch_file) + "_fail.mcl") diff_mcl_file = os.path.join(temp_location, os.path.basename(mch_file) + "_diff.mcl") + base_metrics_summary_file = os.path.join(temp_location, os.path.basename(mch_file) + "_base_metrics.csv") + diff_metrics_summary_file = os.path.join(temp_location, os.path.basename(mch_file) + "_diff_metrics.csv") + # If the user passed -temp_dir, we skip the SuperPMI replay process, # and rely on what we find from a previous run. if self.coreclr_args.temp_dir is not None: @@ -1810,7 +1830,9 @@ def replay_with_asm_diffs(self): "-v", "ewmi", # display errors, warnings, missing, jit info "-f", fail_mcl_file, # Failing mc List "-diffMCList", diff_mcl_file, # Create all of the diffs in an mcl file - "-r", os.path.join(temp_location, "repro") # Repro name, create .mc repro files + "-r", os.path.join(temp_location, "repro"), # Repro name, create .mc repro files + "-baseMetricsSummary", base_metrics_summary_file, # Create summary of metrics we can use to get total code size impact + "-diffMetricsSummary", diff_metrics_summary_file, ] flags += altjit_asm_diffs_flags flags += base_option_flags @@ -1839,7 +1861,7 @@ def replay_with_asm_diffs(self): return_code = run_and_log(command) print_superpmi_failure_code(return_code, self.coreclr_args) if return_code == 0: - logging.info("Clean SuperPMI replay") + logging.info("Clean SuperPMI diff") else: result = False # Don't report as replay failure asm diffs (return code 2) or missing data (return code 3). @@ -1961,6 +1983,17 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: logging.debug(item) logging.debug("") + base_metrics = read_csv_metrics(base_metrics_summary_file) + diff_metrics = read_csv_metrics(diff_metrics_summary_file) + + if base_metrics is not None and "Code bytes" in base_metrics and diff_metrics is not None and "Code bytes" in diff_metrics: + base_bytes = int(base_metrics["Code bytes"]) + diff_bytes = int(diff_metrics["Code bytes"]) + logging.info("Total Code bytes of base: {}".format(base_bytes)) + logging.info("Total Code bytes of diff: {}".format(diff_bytes)) + delta_bytes = diff_bytes - base_bytes + logging.info("Total Code bytes of delta: {} ({:.2%} of base)".format(delta_bytes, delta_bytes / base_bytes)) + try: current_text_diff = text_differences.get_nowait() except: @@ -1985,6 +2018,9 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: command = [ jit_analyze_path, "--md", md_summary_file, "-r", "--base", base_asm_location, "--diff", diff_asm_location ] if self.coreclr_args.metrics: command += [ "--metrics", ",".join(self.coreclr_args.metrics) ] + elif base_bytes is not None and diff_bytes is not None: + command += [ "--override-total-base-metric", str(base_bytes), "--override-total-diff-metric", str(diff_bytes) ] + run_and_log(command, logging.INFO) ran_jit_analyze = True @@ -2015,6 +2051,14 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: os.remove(fail_mcl_file) fail_mcl_file = None + if os.path.isfile(base_metrics_summary_file): + os.remove(base_metrics_summary_file) + base_metrics_summary_file = None + + if os.path.isfile(diff_metrics_summary_file): + os.remove(diff_metrics_summary_file) + diff_metrics_summary_file = None + ################################################################################################ end of for mch_file in self.mch_files # Report the overall results summary of the asmdiffs run From 38b91766bb1192662aa95f3a13f23ad44f80b3b1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Oct 2021 16:19:35 +0200 Subject: [PATCH 2/4] Adapt to use PAL --- .../superpmi/superpmi-shared/standardpch.h | 2 + .../ToolBox/superpmi/superpmi/CMakeLists.txt | 3 + .../superpmi/superpmi/metricssummary.cpp | 68 +++++++++++++++---- .../superpmi/superpmi/metricssummary.h | 4 +- src/coreclr/inc/clr_std/vector | 5 ++ 5 files changed, 67 insertions(+), 15 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/standardpch.h b/src/coreclr/ToolBox/superpmi/superpmi-shared/standardpch.h index 731e8305da6954..1dfae365bc4dbc 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/standardpch.h +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/standardpch.h @@ -68,12 +68,14 @@ #ifdef TARGET_UNIX #include "clr_std/string" #include "clr_std/algorithm" +#include "clr_std/vector" #else // !TARGET_UNIX #ifndef USE_STL #define USE_STL #endif // USE_STL #include #include +#include #endif // !TARGET_UNIX #ifdef USE_MSVCDIS diff --git a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt index 4827c195dc2997..792dc0c052539e 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt +++ b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt @@ -12,6 +12,9 @@ if(CLR_CMAKE_HOST_WIN32) set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) endif(CLR_CMAKE_HOST_WIN32) +set(CMAKE_CXX_STANDARD 11) +set(CMAKE_CXX_STANDARD_REQUIRED ON) + include_directories(.) include_directories(../superpmi-shared) diff --git a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp index 0b5a620b5b0d5c..a55119c6a96ea4 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp @@ -5,38 +5,80 @@ #include "metricssummary.h" #include "logging.h" -#include -#include +struct HandleCloser +{ + void operator()(HANDLE hFile) + { + CloseHandle(hFile); + } +}; + +struct FileHandleWrapper +{ + FileHandleWrapper(HANDLE hFile) + : hFile(hFile) + { + } + + ~FileHandleWrapper() + { + CloseHandle(hFile); + } + + HANDLE get() { return hFile; } + +private: + HANDLE hFile; +}; bool MetricsSummary::SaveToFile(const char* path) { - FILE* file = fopen(path, "w"); - if (file == nullptr) + FileHandleWrapper file(CreateFile(path, GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + if (file.get() == INVALID_HANDLE_VALUE) + { + return false; + } + + char buffer[4096]; + int len = + sprintf_s(buffer, sizeof(buffer), "Successful compiles,Failing compiles,Code bytes\n%d,%d,%lld\n", + SuccessfulCompiles, FailingCompiles, NumCodeBytes); + DWORD numWritten; + if (!WriteFile(file.get(), buffer, static_cast(len), &numWritten, nullptr) || numWritten != len) { return false; } - fprintf(file, "Successful compiles,Failing compiles,Code bytes\n"); - fprintf(file, "%d,%d,%" PRId64 "\n", SuccessfulCompiles, FailingCompiles, NumCodeBytes); - fclose(file); return true; } -bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* result) +bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* metrics) { - FILE* file = fopen(path, "r"); - if (file == nullptr) + FileHandleWrapper file(CreateFile(path, GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)); + if (file.get() == INVALID_HANDLE_VALUE) { return false; } - fscanf(file, "Successful compiles,Failing compiles,Code bytes\n"); - if (fscanf(file, "%d,%d,%" SCNd64 "\n", &result->SuccessfulCompiles, &result->FailingCompiles, &result->NumCodeBytes) != 3) + LARGE_INTEGER len; + if (!GetFileSizeEx(file.get(), &len)) + { + return false; + } + + std::vector content(static_cast(len.QuadPart)); + DWORD numRead; + if (!ReadFile(file.get(), content.data(), static_cast(content.size()), &numRead, nullptr) || numRead != content.size()) + { + return false; + } + + if (sscanf_s(content.data(), "Successful compiles,Failing compiles,Code bytes\n%d,%d,%lld\n", + &metrics->SuccessfulCompiles, &metrics->FailingCompiles, &metrics->NumCodeBytes) != 3) { return false; } - fclose(file); return true; } diff --git a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h index 6ed0922152ec65..4f7d825c2d43aa 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h +++ b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h @@ -9,7 +9,7 @@ class MetricsSummary public: int SuccessfulCompiles; int FailingCompiles; - int64_t NumCodeBytes; + long long NumCodeBytes; MetricsSummary() : SuccessfulCompiles(0) @@ -19,7 +19,7 @@ class MetricsSummary } bool SaveToFile(const char* path); - static bool LoadFromFile(const char* path, MetricsSummary* result); + static bool LoadFromFile(const char* path, MetricsSummary* metrics); void AggregateFrom(const MetricsSummary& other); }; diff --git a/src/coreclr/inc/clr_std/vector b/src/coreclr/inc/clr_std/vector index 0bed04182a4ee6..c10ecf6ba5a92c 100644 --- a/src/coreclr/inc/clr_std/vector +++ b/src/coreclr/inc/clr_std/vector @@ -373,6 +373,11 @@ namespace std m_size -= elements; return iterator(m_pelements + (position - begin())); } + + T* data() + { + return m_pelements; + } const T* data() const { From b049852bda1d4b3811d8513e615368d0499b1dca Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Oct 2021 17:18:43 +0200 Subject: [PATCH 3/4] Remove unnecessary makefile change --- src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt index 792dc0c052539e..4827c195dc2997 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt +++ b/src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt @@ -12,9 +12,6 @@ if(CLR_CMAKE_HOST_WIN32) set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) endif(CLR_CMAKE_HOST_WIN32) -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) - include_directories(.) include_directories(../superpmi-shared) From 55771026aed009138f0530bf8b1101c34029f38d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Oct 2021 18:05:39 +0200 Subject: [PATCH 4/4] Fix GCC build --- src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp index a55119c6a96ea4..e7c725d7845bf9 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp @@ -44,7 +44,7 @@ bool MetricsSummary::SaveToFile(const char* path) sprintf_s(buffer, sizeof(buffer), "Successful compiles,Failing compiles,Code bytes\n%d,%d,%lld\n", SuccessfulCompiles, FailingCompiles, NumCodeBytes); DWORD numWritten; - if (!WriteFile(file.get(), buffer, static_cast(len), &numWritten, nullptr) || numWritten != len) + if (!WriteFile(file.get(), buffer, static_cast(len), &numWritten, nullptr) || numWritten != static_cast(len)) { return false; }