JIT: Add a unified mechanism to track metadata/metrics about the compilation #98653

jakobbotsch · 2024-02-19T13:09:38Z

Adds a JIT-EE API to report metadata back to the EE side about the JIT compilation, and adds support for saving these metrics as part of SPMI runs. Switches a number of adhoc metrics to use this scheme, and also adds a few new ones.

The metadata is currently only reported with checked JITs.

Also adds support for fast perfscore diffs, and adds perfscore into the reports generated by superpmi.py asmdiffs.

Fix #52877

As future work we should include the metrics as part of the generated diffs report. I think we can do that automatically with relatively little effort. We also may want some variant of superpmi.py replay which just collects the metrics into a report.

The system also reports back the method full name and the tiering name as metadata. We can use this in the future to improve the analysis (in particular, grouping repeated diffs in methods of the same full name + tiering name).

For Andy's use case we may consider reporting the metrics back to the EE even in release builds (maybe only under JitMetrics or something else that SPMI could set).

ghost · 2024-02-19T13:09:57Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Adds a JIT-EE API to report metadata back to the EE side about the JIT compilation, and adds support for saving these metrics as part of SPMI runs. Switches a number of adhoc metrics to use this scheme.

Fix #52877

Author:	jakobbotsch
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

…ilation Adds a JIT-EE API to report metadata back to the EE side about the JIT compilation, and adds support for saving these metrics as part of SPMI runs. Switches a number of adhoc metrics to use this scheme.

jakobbotsch · 2024-02-19T15:24:32Z

src/coreclr/tools/superpmi/superpmi/fileio.cpp

+bool FileWriter::PrintQuotedCsvField(const char* value)
+{
+    size_t numQuotes = 0;
+    for (const char* p = value; *p != '\0'; p++)
+    {
+        if (*p == '"')
+        {
+            numQuotes++;
+        }
+    }
+
+    if (numQuotes == 0)
+    {
+        return Printf("\"%s\"", value);
+    }
+    else
+    {
+        size_t len = 2 + strlen(value) + numQuotes;
+        char* buffer = new char[len];
+
+        size_t index = 0;
+        buffer[index++] = '"';
+        for (const char* p = value; *p != '\0'; p++)
+        {
+            if (*p == '"')
+            {
+                buffer[index++] = '"';
+            }
+            buffer[index++] = *p;
+        }
+
+        buffer[index++] = '"';
+        assert(index == len);
+
+        bool result = Print(buffer, len);
+        delete[] buffer;
+        return result;
+    }
+}


Oddly there's a function with full name

System.Xml.Xsl.CompiledQuery.Query:<xsl:template match="Class">(System.Xml.Xsl.Runtime.XmlQueryRuntime,System.Xml.XPath.XPathNavigator,double,double,System.Collections.Generic.IList`1[System.Xml.XPath.XPathNavigator])

in libraries_tests.run, so I had to add this support.

… the end

jakobbotsch · 2024-02-19T21:39:13Z

src/coreclr/pal/src/safecrt/vsprintf.cpp

@@ -95,7 +95,7 @@ DLLEXPORT int __cdecl _vsnprintf_s (
        retvalue = vsnprintf(string, sizeInBytes, format, ap);
        string[sizeInBytes - 1] = '\0';
        /* we allow truncation if count == _TRUNCATE */
-        if (retvalue > (int)sizeInBytes && count == _TRUNCATE)
+        if (retvalue >= (int)sizeInBytes && count == _TRUNCATE)


Will submit this fix separately... it should have a unit test added as well.

jakobbotsch · 2024-02-20T18:44:10Z

cc @dotnet/jit-contrib PTAL @BruceForstall @AndyAyersMS

Here is an example for how the new SPMI report looks. I added the "PerfScore in Diffs" column to the main result tables and a "PerfScore Overall (FullOpts)" column in the "Details" tables. The former one specifies the geomean computed over the relative perfscore in every context with diffs; the latter one is the geomean computed over the relative perfscore for all contexts. In other words, the one shown in the main table can be interpreted as "when my optimization kicks in, how much does it affect perf score". The one in the details is more of a "what is the overall impact".

BruceForstall

LGTM. Some questions/suggestions.

One additional question: should the metadata info list / schema be statically known (defined in a header file), or dynamic? (clients don't know all the metadata names/types, except maybe by convention, beforehand)

BruceForstall · 2024-02-20T23:46:32Z

src/coreclr/jit/jitmetadatalist.h

+JITMETADATAMETRIC(LoopsAligned,                int,              0)
+JITMETADATAMETRIC(VarsInSsa,                   int,              0)
+JITMETADATAMETRIC(HoistedExpressions,          int,              0)
+JITMETADATAMETRIC(RedundantBranchesEliminated, int,              JIT_METADATA_HIGHER_IS_BETTER)


Are the flags used anywhere? Where is JIT_METADATA_HIGHER_IS_BETTER defined? It seems odd to include this file in superpmi source code but not have this defined there.

They aren't yet, but my plan was to use this to automatically colorize the metrics in the report (once I add that support). Indeed I haven't defined the enum anywhere yet (and it'll probably only end up being define in superpmi and not the JIT).

src/coreclr/jit/jitmetadatalist.h

BruceForstall · 2024-02-20T23:48:28Z

src/coreclr/jit/jitmetadatalist.h

+JITMETADATAINFO(MethodFullName,                const char*,      0)
+JITMETADATAINFO(TieringName,                   const char*,      0)


What is the difference between JITMETADATAINFO and JITMETADATAMETRIC? (Document above).

Also, the answers to MethodFullName and TieringName are hard coded in SPMI playback. Should that be noted somewhere?

Added this comment above:

// List of metadata that the JIT can report. There are two categories: // // - JITMETADATAINFO: General info that can be of any type and that cannot be // aggregated in straightforward ways. These properties are not handled // automatically; the JIT must explicitly report them using // JitMetadata::report, and the SPMI side needs to manually handle (or ignore) // them in ICorJitInfo::reportMetadata. // // - JITMETADATAMETRIC: Metrics which are numeric types (currently int, double // and int64_t types supported). Their reporting is handled automatically and // they will be propagated all the way into SPMI replay/diff results.

BruceForstall · 2024-02-20T23:50:48Z

src/coreclr/jit/jitmetadata.cpp

+//
+void JitMetadata::report(Compiler* comp, JitMetadataName name, const void* data)
+{
+    comp->info.compCompHnd->reportMetadata(getName(name), data);


Should the report functions do nothing if RunningSuperPmiReplay() is false? (I guess the VM does nothing anyway)

Yeah I figured that we should just let it be up to the EE. If the EE wants to do something with the information then that's up to them. (Also, we don't check for SPMI in release JITs)

AndyAyersMS

Looks good, Bruce has already covered anything I'd have commented on.

I would like to update JitMetrics to use a variant of this, but there are some vector-valued things there. Hmm.

jakobbotsch · 2024-02-21T09:34:55Z

One additional question: should the metadata info list / schema be statically known (defined in a header file), or dynamic? (clients don't know all the metadata names/types, except maybe by convention, beforehand)

I really want the SPMI side to be able to know what metrics to expect beforehand, just because it simplifies things in the csv handling and also allows e.g. superpmi.py to query metadata about the metrics separately, like the "higher is better" info. There may also be other kinds of metadata about the metrics in the future (for example, some metrics make sense to aggregate as sums/averages, while some other metrics may make more sense to aggregate as min/max). I'd like to be able to have one place to define all of this and automatically have it propagate all the way into superpmi.py -- and I was trying to make jitmetadatalist.h that place.

Initially I was going to add a new method to the host interface to be able to get the information about the metrics dynamically. For example, then the JIT side would be the only place we used jitmetadatalist.h, to report this information back. It would probably be slightly more flexible than the current approach, which has some downsides. For example, in the current approach if you change the type of a metric then running superpmi.exe with an old JIT will make it misinterpret the metric data as the new type when the old JIT is reports the old type. However, it seemed much easier to just use jitmetadatalist.h from SPMI always, and I would expect the problematic changes to jitmetadatalist.h to be rare (and if we need to make them we always have the hammer of updating the JIT-EE GUID).

I would like to update JitMetrics to use a variant of this, but there are some vector-valued things there. Hmm.

I added a size_t length arg to the API. That seems like good API hygiene regardless when dealing with void*, and you can use it for your case to report the full array of metadata in one shot (although you're still going to need some manual handling on the SPMI side, but I guess that's inevitable).

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 19, 2024

ghost assigned jakobbotsch Feb 19, 2024

jakobbotsch added 6 commits February 19, 2024 14:51

JIT: Add a unified mechanism to track metadata/metrics about the comp…

b8a6343

…ilation Adds a JIT-EE API to report metadata back to the EE side about the JIT compilation, and adds support for saving these metrics as part of SPMI runs. Switches a number of adhoc metrics to use this scheme.

Clean ups

6e3cf3e

Nit

4f7f361

Further fixing

ad02f9f

Fixes

4018a4c

Allow access in release as well

a06d6ed

jakobbotsch force-pushed the jit-metrics branch from 047f268 to a06d6ed Compare February 19, 2024 14:07

jakobbotsch added 3 commits February 19, 2024 15:47

Support writing CSV field names with quotes in them

867374d

Fix gcc build

3bf7adc

Clean up

e821bbd

jakobbotsch commented Feb 19, 2024

View reviewed changes

jakobbotsch added 7 commits February 19, 2024 17:02

Add some more metrics; display them in JITDUMP; really report them at…

1e2a1d9

… the end

Reorder a bit

d96f2e5

Nit

469c3de

Update JIT-EE version GUID again

601cf6b

Support fast PerfScore diffs in superpmi.py

186da27

Fix reporting inlinee full name

1a075c3

Fix a bug in PAL version of _vsnprintf_s

a637694

jakobbotsch commented Feb 19, 2024

View reviewed changes

jakobbotsch added 7 commits February 20, 2024 14:02

Clean up, add function header comment

4624ddc

Fix

7dd107c

Print PerfScore geomeans

eca3dd9

Add to report

f890303

Clean up

ee5ad8b

Move overall perfscore change to details

a839f53

Merge branch 'main' of github.com:dotnet/runtime into jit-metrics

df8f962

jakobbotsch marked this pull request as ready for review February 20, 2024 18:40

jakobbotsch requested a review from MichalStrehovsky as a code owner February 20, 2024 18:40

jakobbotsch requested review from AndyAyersMS and BruceForstall February 20, 2024 18:44

BruceForstall approved these changes Feb 20, 2024

View reviewed changes

AndyAyersMS approved these changes Feb 21, 2024

View reviewed changes

Address feedback

845bb9c

jakobbotsch merged commit 80084aa into dotnet:main Feb 21, 2024
121 of 125 checks passed

jakobbotsch deleted the jit-metrics branch February 21, 2024 10:40

github-actions bot locked and limited conversation to collaborators Mar 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Add a unified mechanism to track metadata/metrics about the compilation #98653

JIT: Add a unified mechanism to track metadata/metrics about the compilation #98653

jakobbotsch commented Feb 19, 2024 •

edited

Loading

ghost commented Feb 19, 2024

jakobbotsch Feb 19, 2024

jakobbotsch Feb 19, 2024

jakobbotsch Feb 20, 2024

jakobbotsch commented Feb 20, 2024

BruceForstall left a comment

BruceForstall Feb 20, 2024

jakobbotsch Feb 21, 2024

BruceForstall Feb 20, 2024

jakobbotsch Feb 21, 2024

BruceForstall Feb 20, 2024

jakobbotsch Feb 21, 2024

AndyAyersMS left a comment

jakobbotsch commented Feb 21, 2024 •

edited

Loading

		JITMETADATAINFO(MethodFullName, const char*, 0)
		JITMETADATAINFO(TieringName, const char*, 0)

JIT: Add a unified mechanism to track metadata/metrics about the compilation #98653

JIT: Add a unified mechanism to track metadata/metrics about the compilation #98653

Conversation

jakobbotsch commented Feb 19, 2024 • edited Loading

ghost commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Feb 20, 2024

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

jakobbotsch commented Feb 21, 2024 • edited Loading

jakobbotsch commented Feb 19, 2024 •

edited

Loading

jakobbotsch commented Feb 21, 2024 •

edited

Loading