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.

Sign up for GitHub

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

Jump to bottom

Support passing total code size through when doing SPMI diffs #60124

Merged
merged 4 commits into from
Oct 8, 2021
Merged

Support passing total code size through when doing SPMI diffs #60124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
993511b
Support passing total code size through when doing SPMI diffs
jakobbotsch Oct 7, 2021
38b9176
Adapt to use PAL
jakobbotsch Oct 7, 2021
b049852
Remove unnecessary makefile change
jakobbotsch Oct 7, 2021
5577102
Fix GCC build
jakobbotsch Oct 7, 2021
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
2 changes: 2 additions & 0 deletions src/coreclr/ToolBox/superpmi/superpmi-shared/standardpch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <string>
#include <algorithm>
#include <vector>
#endif // !TARGET_UNIX

#ifdef USE_MSVCDIS
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ set(SUPERPMI_SOURCES
jitdebugger.cpp
jitinstance.cpp
methodstatsemitter.cpp
metricssummary.cpp
neardiffer.cpp
parallelsuperpmi.cpp
superpmi.cpp
Expand Down
30 changes: 30 additions & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/commandline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <file name>, -baseMetricsSummary <file name.csv>\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 <file name>\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");
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/commandline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class CommandLine
, indexes(nullptr)
, hash(nullptr)
, methodStatsTypes(nullptr)
, baseMetricsSummaryFile(nullptr)
, diffMetricsSummaryFile(nullptr)
, mclFilename(nullptr)
, diffMCLFilename(nullptr)
, targetArchitecture(nullptr)
Expand Down Expand Up @@ -67,6 +69,8 @@ class CommandLine
int* indexes;
char* hash;
char* methodStatsTypes;
char* baseMetricsSummaryFile;
char* diffMetricsSummaryFile;
char* mclFilename;
char* diffMCLFilename;
char* targetArchitecture;
Expand Down
10 changes: 9 additions & 1 deletion src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "jithost.h"
#include "errorhandling.h"
#include "spmiutil.h"
#include "metricssummary.h"

JitInstance* JitInstance::InitJit(char* nameOfJit,
bool breakOnAssert,
Expand Down Expand Up @@ -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
{
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

@BruceForstall Yes, but if it passes in "base" then we write its size to NumCodeBytes and we don't know that it will fail in diff, so we ignore asm files as you say but this new metric does not look at asm files if I read the change correctly.

So we pass --override-total-base-metric as total number of bytes from compiled methods with base and --override-total-diff-metric as total number of bytes from compiled methods with diff.

Copy link
Member

Choose a reason for hiding this comment

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

I see, we actually are adding the code size before we do the asmdiff. It seems like we should delay until after InvokeNearDiffer is successful (either shows no diffs or some diffs, but doesn't crash) before adding the code size. @jakobbotsch ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that might make sense. But if there are misses then posting the summary is misleading in any case because it does not show any diff in those methods, which is probably where the diffs will actually be. So at least this way it is visible to me that there are roughly 7 KB of code that SPMI misses now and that you should probably post PMI diffs instead.

I can change it but I don't see how we can make this case not be misleading to the reader of the summary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it has to be wordy, like

Total metric of base: 7178021 bytes, 101 methods;
Total metric of diff: 7171065 bytes, 100 methods;
Total metric of delta: -6956 bytes differs (-0.10% of base), -1 method (-1% of base)
Total metric of delta in matching methods: +100 (+0.1 of base) on 100 methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the numbers are not useful, since what I would be interested in as a reader is the diffs in the failed methods. The idea behind this change was to get a better overview of how the change affects libraries crossgen/PMI as a whole and we can never get that when there's missing data. When I see the "Missing data encountered" I always use PMI for my diffs instead.

Copy link
Member Author

@jakobbotsch jakobbotsch Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could sum the code bytes in the dasm files in jit-analyze and at least give a clear warning that they do not account for all diffs. But this only helps if someone does not notice that SPMI diffs are only partial and then posts the results anyway, not sure if this is what we're afraid of?
Are there are cases where we know there won't be diffs in the functions with missing data encountered so that the SPMI diffs give an accurate view anyway? I can see keeping track of a "number of diffed code bytes" metric as @BruceForstall suggested for this case, but in other cases it seems we should not use SPMI for diffs.

}
else
{
LogDebug("compileMethod failed with result %d", jitResult);
pParam->result = RESULT_ERROR;

pParam->metrics->FailingCompiles++;
}
}
PAL_EXCEPT_FILTER(FilterSuperPMIExceptions_CaptureExceptionAndStop)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/ToolBox/superpmi/superpmi/jitinstance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/methodstatsemitter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ class MethodStatsEmitter
void Emit(int methodNumber, MethodContext* mc, ULONGLONG firstTime, ULONGLONG secondTime);
void SetStatsTypes(char* types);
};

#endif
90 changes: 90 additions & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/metricssummary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// 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"

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)
{
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<DWORD>(len), &numWritten, nullptr) || numWritten != static_cast<DWORD>(len))
{
return false;
}

return true;
}

bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* metrics)
{
FileHandleWrapper file(CreateFile(path, GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr));
if (file.get() == INVALID_HANDLE_VALUE)
{
return false;
}

LARGE_INTEGER len;
if (!GetFileSizeEx(file.get(), &len))
{
return false;
}

std::vector<char> content(static_cast<size_t>(len.QuadPart));
DWORD numRead;
if (!ReadFile(file.get(), content.data(), static_cast<DWORD>(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;
}

return true;
}

void MetricsSummary::AggregateFrom(const MetricsSummary& other)
{
SuccessfulCompiles += other.SuccessfulCompiles;
FailingCompiles += other.FailingCompiles;
NumCodeBytes += other.NumCodeBytes;
}
26 changes: 26 additions & 0 deletions src/coreclr/ToolBox/superpmi/superpmi/metricssummary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
long long NumCodeBytes;

MetricsSummary()
: SuccessfulCompiles(0)
, FailingCompiles(0)
, NumCodeBytes(0)
{
}

bool SaveToFile(const char* path);
static bool LoadFromFile(const char* path, MetricsSummary* metrics);
void AggregateFrom(const MetricsSummary& other);
};

#endif
Loading