From 706953dc533628cd04e8b6dc91da590884e1bb09 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 25 Aug 2022 16:47:22 +0200 Subject: [PATCH 01/22] Add some artificial diffs --- src/coreclr/jit/forwardsub.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index 6981939198fee3..6ee56015106ea7 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -114,6 +114,15 @@ PhaseStatus Compiler::fgForwardSub() { return PhaseStatus::MODIFIED_NOTHING; } + else + { + CLRRandom rng; + rng.Init(info.compMethodHash() ^ 0x12345678); + if (rng.Next(10) == 0) + { + return PhaseStatus::MODIFIED_NOTHING; + } + } #endif bool changed = false; From ba7bd0c5498017151401a5c0b02afe7224c9aeb5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 25 Aug 2022 16:51:47 +0200 Subject: [PATCH 02/22] JIT: Enhance metrics reported by superpmi diffs * Report the total number of contexts, minopts contexts and fullopts contexted processed * Report number of successful and missing contexts * Report asmdiffs and tpdiffs for minopts/fullopts separately Fixes #70350 Contributes to #73506 --- src/coreclr/scripts/superpmi.py | 200 +++++++++++++----- .../scripts/superpmi_diffs_summarize.py | 35 +-- .../superpmi-shared/methodcontext.cpp | 1 + .../tools/superpmi/superpmi/jitinstance.cpp | 6 +- .../tools/superpmi/superpmi/jitinstance.h | 2 +- .../superpmi/superpmi/metricssummary.cpp | 173 ++++++++++----- .../tools/superpmi/superpmi/metricssummary.h | 20 +- .../superpmi/superpmi/parallelsuperpmi.cpp | 24 ++- .../tools/superpmi/superpmi/superpmi.cpp | 53 +++-- 9 files changed, 349 insertions(+), 165 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 9e30bf5e77cfb6..cf15911a984abb 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -478,21 +478,25 @@ def create_artifacts_base_name(coreclr_args, mch_file): def read_csv_metrics(path): - """ Read a metrics summary file produced by superpmi, and return the single row containing the information as a dictionary. + """ Read a metrics summary file produced by superpmi and return the rows as a dictionary of dictionaries. Args: path (str) : path to .csv file Returns: - A dictionary with each metric + A dictionary of dictionaries. For example, dict["Total"]["Successful + compiles"] will access the total number of successful compiles and + dict["MinOpts"]["Successful compiles"] will access the number of + minopts compilations. """ + dict = {} with open(path) as csv_file: reader = csv.DictReader(csv_file) for row in reader: - return row + dict[row["Name"]] = row - return None + return dict def determine_clrjit_compiler_version(clrjit_path): """ Obtain the version of the compiler that was used to compile the clrjit at the specified path. @@ -1188,7 +1192,7 @@ def print_superpmi_result(return_code, coreclr_args, base_metrics, diff_metrics) those return codes. """ if return_code == 0: - logging.info("Clean SuperPMI {} ({} contexts processed)".format("replay" if diff_metrics is None else "diff", base_metrics["Successful compiles"])) + logging.info("Clean SuperPMI {} ({} contexts processed)".format("replay" if diff_metrics is None else "diff", base_metrics["Total"]["Successful compiles"])) elif return_code == -1 or return_code == 4294967295: logging.error("General fatal error") elif return_code == -2 or return_code == 4294967294: @@ -1198,13 +1202,13 @@ def print_superpmi_result(return_code, coreclr_args, base_metrics, diff_metrics) elif return_code == 2: logging.warning("Asm diffs found") elif return_code == 3: - missing_base = int(base_metrics["Missing compiles"]) - total_contexts = int(base_metrics["Successful compiles"]) + int(base_metrics["Failing compiles"]) + missing_base = int(base_metrics["Total"]["Missing compiles"]) + total_contexts = int(base_metrics["Total"]["Successful compiles"]) + int(base_metrics["Total"]["Failing compiles"]) if diff_metrics is None: logging.warning("SuperPMI encountered missing data for {} out of {} contexts".format(missing_base, total_contexts)) else: - missing_diff = int(diff_metrics["Missing compiles"]) + missing_diff = int(diff_metrics["Total"]["Missing compiles"]) logging.warning("SuperPMI encountered missing data. Missing with base JIT: {}. Missing with diff JIT: {}. Total contexts: {}.".format(missing_base, missing_diff, total_contexts)) elif return_code == 139 and coreclr_args.host_os != "windows": @@ -1521,7 +1525,7 @@ def replay_with_asm_diffs(self): files_with_replay_failures = [] # List of all Markdown summary files - all_md_summary_files = [] + asm_diffs = [] with TempDir(None, self.coreclr_args.skip_cleanup) as temp_location: logging.debug("") @@ -1606,13 +1610,16 @@ def replay_with_asm_diffs(self): save_repro_mc_files(temp_location, self.coreclr_args, artifacts_base_name, repro_base_command_line) # This file had asm diffs; keep track of that. - if is_nonzero_length_file(diff_mcl_file): + has_diffs = is_nonzero_length_file(diff_mcl_file) + if has_diffs: files_with_asm_diffs.append(mch_file) # There were diffs. Go through each method that created diffs and - # create a base/diff asm file with diffable asm. In addition, create - # a standalone .mc for easy iteration. - if is_nonzero_length_file(diff_mcl_file) and not self.coreclr_args.diff_with_release: + # create a base/diff asm file with diffable asm so we can analyze + # it. In addition, create a standalone .mc for easy iteration. + jit_analyze_summary_file = None + + if has_diffs and not self.coreclr_args.diff_with_release: # AsmDiffs. Save the contents of the fail.mcl file to dig into failures. if return_code == 0: @@ -1711,8 +1718,8 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: logging.debug("") if base_metrics is not None and diff_metrics is not None: - base_bytes = int(base_metrics["Diffed code bytes"]) - diff_bytes = int(diff_metrics["Diffed code bytes"]) + base_bytes = int(base_metrics["Total"]["Diffed code bytes"]) + diff_bytes = int(diff_metrics["Total"]["Diffed code bytes"]) logging.info("Total bytes of base: {}".format(base_bytes)) logging.info("Total bytes of diff: {}".format(diff_bytes)) delta_bytes = diff_bytes - base_bytes @@ -1736,10 +1743,8 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: jit_analyze_path = find_file(jit_analyze_file, path_var.split(os.pathsep)) if jit_analyze_path is not None: # It appears we have a built jit-analyze on the path, so try to run it. - md_summary_file = os.path.join(asm_root_dir, "summary.md") - summary_file_info = ( mch_file, md_summary_file ) - all_md_summary_files.append(summary_file_info) - command = [ jit_analyze_path, "--md", md_summary_file, "-r", "--base", base_asm_location, "--diff", diff_asm_location ] + jit_analyze_summary_file = os.path.join(asm_root_dir, "summary.md") + command = [ jit_analyze_path, "--md", jit_analyze_summary_file, "-r", "--base", base_asm_location, "--diff", diff_asm_location ] if self.coreclr_args.retainOnlyTopFiles: command += [ "--retainOnlyTopFiles" ] if self.coreclr_args.metrics: @@ -1785,15 +1790,18 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: logging.warning("No textual differences found in generated JitDump. Is this an issue with coredistools?") if base_metrics is not None and diff_metrics is not None: - missing_base = int(base_metrics["Missing compiles"]) - missing_diff = int(diff_metrics["Missing compiles"]) - total_contexts = int(base_metrics["Successful compiles"]) + int(base_metrics["Failing compiles"]) + missing_base = int(base_metrics["Total"]["Missing compiles"]) + missing_diff = int(diff_metrics["Total"]["Missing compiles"]) + total_contexts = int(base_metrics["Total"]["Successful compiles"]) + int(base_metrics["Total"]["Failing compiles"]) if missing_base > 0 or missing_diff > 0: logging.warning("Warning: SuperPMI encountered missing data during the diff. The diff summary printed above may be misleading.") logging.warning("Missing with base JIT: {}. Missing with diff JIT: {}. Total contexts: {}.".format(missing_base, missing_diff, total_contexts)) - ################################################################################################ end of processing asm diffs (if is_nonzero_length_file(diff_mcl_file)... + ################################################################################################ end of processing asm diffs (if has_diffs... + + mch_file_basename = os.path.basename(mch_file) + asm_diffs.append((mch_file_basename, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file)) if not self.coreclr_args.skip_cleanup: if os.path.isfile(fail_mcl_file): @@ -1816,7 +1824,7 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: # Construct an overall Markdown summary file. - if len(all_md_summary_files) > 0 and not self.coreclr_args.diff_with_release: + if len(asm_diffs) > 0 and not self.coreclr_args.diff_with_release: overall_md_summary_file = create_unique_file_name(self.coreclr_args.spmi_location, "diff_summary", "md") if not os.path.isdir(self.coreclr_args.spmi_location): os.makedirs(self.coreclr_args.spmi_location) @@ -1824,13 +1832,89 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: os.remove(overall_md_summary_file) with open(overall_md_summary_file, "w") as write_fh: - for summary_file_info in all_md_summary_files: - summary_mch = summary_file_info[0] - summary_mch_filename = os.path.basename(summary_mch) # Display just the MCH filename, not the full path - summary_file = summary_file_info[1] - with open(summary_file, "r") as read_fh: - write_fh.write("## " + summary_mch_filename + ":\n\n") - shutil.copyfileobj(read_fh, write_fh) + def format_delta(base, diff): + plus_if_positive = "+" if diff >= base else "" + pct = "" + if base > 0: + pct = " ({}{:.2f}%)".format(plus_if_positive, (diff - base) / base * 100) + + return "{}{:,d} B{}".format(plus_if_positive, diff - base, pct) + + diffed_contexts = sum(int(diff_metrics["Total"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + diffed_opts_contexts = sum(int(diff_metrics["Opts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + missing_base_contexts = sum(int(base_metrics["Total"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) + missing_diff_contexts = sum(int(diff_metrics["Total"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + + write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n".format( + diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) + + write_fh.write("{:,d} contexts missed with the base JIT and {:,d} missed with the diff JIT.\n\n".format(missing_base_contexts, missing_diff_contexts)) + + if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): + # First write an overview table + write_fh.write("|Collection|Overall|MinOpts|Opts|\n") + write_fh.write("|---|---|---|---|\n") + for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: + if not has_diffs: + continue + + write_fh.write("|{}|{}|{}|{}|\n".format( + mch_file, + format_delta( + int(base_metrics["Total"]["Diffed code bytes"]), + int(diff_metrics["Total"]["Diffed code bytes"])), + format_delta( + int(base_metrics["MinOpts"]["Diffed code bytes"]), + int(diff_metrics["MinOpts"]["Diffed code bytes"])), + format_delta( + int(base_metrics["Opts"]["Diffed code bytes"]), + int(diff_metrics["Opts"]["Diffed code bytes"])))) + + write_fh.write("\n") + else: + write_fh.write("No diffs found.\n") + + # Next write a detailed section + write_fh.write("\n
\n") + write_fh.write("Details\n\n") + + write_fh.write("|Collection|Diffed contexts|Base missing contexts|Diff missing contexts|Minopts contexts|Optimized contexts|\n") + write_fh.write("|---|---|---|---|---|---|\n") + for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: + write_fh.write("|{}|{:,d}|{:,d}|{:,d}|{:,d}|{:,d}|\n".format( + mch_file, + int(diff_metrics["Total"]["Successful compiles"]), + int(base_metrics["Total"]["Missing compiles"]), + int(diff_metrics["Total"]["Missing compiles"]), + int(diff_metrics["MinOpts"]["Successful compiles"]), + int(diff_metrics["Opts"]["Successful compiles"]))) + + write_fh.write("\n") + + if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): + for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: + if not has_diffs or jit_analyze_summary_file is None: + continue + + with open(jit_analyze_summary_file, "r") as read_fh: + write_fh.write("""\ + +
+ +{0} + +To reproduce these diffs on Windows {1}: +``` +superpmi.py asmdiffs -target_os {2} -target_arch {3} -arch {1} +``` + +""".format(mch_file, self.coreclr_args.arch, self.coreclr_args.target_os, self.coreclr_args.target_arch)) + + shutil.copyfileobj(read_fh, write_fh) + write_fh.write("\n
\n") + + write_fh.write("\n
\n") logging.info(" Summary Markdown file: %s", overall_md_summary_file) @@ -1977,15 +2061,15 @@ def replay_with_throughput_diff(self): diff_metrics = read_csv_metrics(diff_metrics_summary_file) if base_metrics is not None and diff_metrics is not None: - base_instructions = int(base_metrics["Diff executed instructions"]) - diff_instructions = int(diff_metrics["Diff executed instructions"]) + base_instructions = int(base_metrics["Total"]["Diff executed instructions"]) + diff_instructions = int(diff_metrics["Total"]["Diff executed instructions"]) logging.info("Total instructions executed by base: {}".format(base_instructions)) logging.info("Total instructions executed by diff: {}".format(diff_instructions)) if base_instructions != 0 and diff_instructions != 0: delta_instructions = diff_instructions - base_instructions logging.info("Total instructions executed delta: {} ({:.2%} of base)".format(delta_instructions, delta_instructions / base_instructions)) - tp_diffs.append((os.path.basename(mch_file), base_instructions, diff_instructions)) + tp_diffs.append((os.path.basename(mch_file), base_metrics, diff_metrics)) else: logging.warning("One compilation failed to produce any results") else: @@ -2024,30 +2108,48 @@ def replay_with_throughput_diff(self): # We write two tables, an overview one with just significantly # impacted collections and a detailed one that includes raw # instruction count and all collections. - def is_significant(base, diff): + def is_significant_pct(base, diff): return round((diff - base) / base * 100, 2) != 0 + def is_significant(base, diff): + def check(col): + return is_significant_pct(int(base[col]["Diff executed instructions"]), int(diff[col]["Diff executed instructions"])) + + return check("Total") or check("MinOpts") or check("Opts") + def format_pct(base_instructions, diff_instructions): + plus_if_positive = "+" if diff_instructions > base_instructions else "" + return "{}{:.2f}%".format(plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) if any(is_significant(base, diff) for (_, base, diff) in tp_diffs): - write_fh.write("|Collection|PDIFF|\n") - write_fh.write("|---|---|\n") - for mch_file, base_instructions, diff_instructions in tp_diffs: - if is_significant(base_instructions, diff_instructions): - write_fh.write("|{}|{}{:.2f}%|\n".format( + write_fh.write("|Collection|Overall|MinOpts|Opts|\n") + write_fh.write("|---|---|---|---|\n") + for mch_file, base, diff in tp_diffs: + def format_pct_for_col(col): + base_instructions = int(base[col]["Diff executed instructions"]) + diff_instructions = int(diff[col]["Diff executed instructions"]) + return format_pct(base_instructions, diff_instructions) + + if is_significant(base, diff): + write_fh.write("|{}|{}|{}|{}|\n".format( mch_file, - "+" if diff_instructions > base_instructions else "", - (diff_instructions - base_instructions) / base_instructions * 100)) + format_pct_for_col("Total"), + format_pct_for_col("MinOpts"), + format_pct_for_col("Opts"))) else: write_fh.write("No significant throughput differences found\n") write_fh.write("\n
\n") write_fh.write("Details\n\n") - write_fh.write("|Collection|Base # instructions|Diff # instructions|PDIFF|\n") - write_fh.write("|---|---|---|---|\n") - for mch_file, base_instructions, diff_instructions in tp_diffs: - write_fh.write("|{}|{:,d}|{:,d}|{}{:.2f}%|\n".format( - mch_file, base_instructions, diff_instructions, - "+" if diff_instructions > base_instructions else "", - (diff_instructions - base_instructions) / base_instructions * 100)) + for (disp, col) in [("All", "Total"), ("MinOpts", "MinOpts"), ("Optimized", "Opts")]: + write_fh.write("{} contexts:\n\n".format(disp)) + write_fh.write("|Collection|Base # instructions|Diff # instructions|PDIFF|\n") + write_fh.write("|---|---|---|---|\n") + for mch_file, base, diff in tp_diffs: + base_instructions = int(base[col]["Diff executed instructions"]) + diff_instructions = int(diff[col]["Diff executed instructions"]) + write_fh.write("|{}|{:,d}|{:,d}|{}|\n".format( + mch_file, base_instructions, diff_instructions, + format_pct(base_instructions, diff_instructions))) + write_fh.write("\n") write_fh.write("\n
\n") logging.info(" Summary Markdown file: %s", overall_md_summary_file) diff --git a/src/coreclr/scripts/superpmi_diffs_summarize.py b/src/coreclr/scripts/superpmi_diffs_summarize.py index 95f3a2cb389f0c..a75d01bd8229e8 100644 --- a/src/coreclr/scripts/superpmi_diffs_summarize.py +++ b/src/coreclr/scripts/superpmi_diffs_summarize.py @@ -89,39 +89,8 @@ def append_diff_file(f, arch, file_name, full_file_path, asmdiffs): else: diffs_found = True f.write("## {} {}\n".format(diff_os, diff_arch)) - - if asmdiffs: - # Contents of asmdiffs are large so create a - #
...
disclosure section around - # the file and additionally provide some instructions. - f.write("""\ - -
- -{0} {1} details - -Summary file: `{2}` - -To reproduce these diffs on Windows {3}: -``` -superpmi.py asmdiffs -target_os {0} -target_arch {1} -arch {3} -``` - -""".format(diff_os, diff_arch, file_name, arch)) - - # Now write the contents - f.write(contents) - - # Write the footer (close the
section) - f.write("""\ - -
- -""") - - else: - f.write(contents) - f.write("\n") + f.write(contents) + f.write("\n") return diffs_found diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index af97ac298c939b..e876dbf4e5dfde 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -1252,6 +1252,7 @@ DWORD MethodContext::repGetJitFlags(CORJIT_FLAGS* jitFlags, DWORD sizeInBytes) DEBUG_REP(dmpGetJitFlags(0, value)); CORJIT_FLAGS* resultFlags = (CORJIT_FLAGS*)GetJitFlags->GetBuffer(value.A); + Assert(sizeInBytes >= value.B); memcpy(jitFlags, resultFlags, value.B); InitReadyToRunFlag(resultFlags); return value.B; diff --git a/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp b/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp index da481413818371..b96fef776fde53 100644 --- a/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp +++ b/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp @@ -298,7 +298,7 @@ extern "C" DLLEXPORT NOINLINE void Instrumentor_GetInsCount(UINT64* result) } } -JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, MetricsSummary* metrics) +JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, MetricsSummary* metrics, CORJIT_FLAGS* jitFlags) { struct Param : FilterSuperPMIExceptionsParam_CaptureException { @@ -309,6 +309,7 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i int mcIndex; bool collectThroughput; MetricsSummary* metrics; + CORJIT_FLAGS* jitFlags; } param; param.pThis = this; param.result = RESULT_SUCCESS; // assume success @@ -316,6 +317,7 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i param.mcIndex = mcIndex; param.collectThroughput = collectThroughput; param.metrics = metrics; + param.jitFlags = jitFlags; // store to instance field our raw values, so we can figure things out a bit later... mc = MethodToCompile; @@ -335,6 +337,7 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i CORINFO_OS os = CORINFO_WINNT; pParam->pThis->mc->repCompileMethod(&pParam->info, &pParam->flags, &os); + pParam->pThis->mc->repGetJitFlags(pParam->jitFlags, sizeof(*pParam->jitFlags)); if (pParam->collectThroughput) { pParam->pThis->lt.Start(); @@ -436,7 +439,6 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i { metrics->SuccessfulCompiles++; metrics->NumExecutedInstructions += static_cast(insCountAfter - insCountBefore); - } else { diff --git a/src/coreclr/tools/superpmi/superpmi/jitinstance.h b/src/coreclr/tools/superpmi/superpmi/jitinstance.h index 8e8fbabf82ead4..7ff4bd4a7004b0 100644 --- a/src/coreclr/tools/superpmi/superpmi/jitinstance.h +++ b/src/coreclr/tools/superpmi/superpmi/jitinstance.h @@ -60,7 +60,7 @@ class JitInstance bool resetConfig(MethodContext* firstContext); - Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, class MetricsSummary* summary); + Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, struct MetricsSummary* metrics, class CORJIT_FLAGS* jitFlags); const WCHAR* getForceOption(const WCHAR* key); const WCHAR* getOption(const WCHAR* key); diff --git a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp index cf93ce902dedd9..64d9c5216c108c 100644 --- a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp +++ b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp @@ -5,13 +5,23 @@ #include "metricssummary.h" #include "logging.h" -struct HandleCloser +void MetricsSummary::AggregateFrom(const MetricsSummary& other) { - void operator()(HANDLE hFile) - { - CloseHandle(hFile); - } -}; + SuccessfulCompiles += other.SuccessfulCompiles; + FailingCompiles += other.FailingCompiles; + MissingCompiles += other.MissingCompiles; + NumCodeBytes += other.NumCodeBytes; + NumDiffedCodeBytes += other.NumDiffedCodeBytes; + NumExecutedInstructions += other.NumExecutedInstructions; + NumDiffExecutedInstructions += other.NumDiffExecutedInstructions; +} + +void MetricsSummaries::AggregateFrom(const MetricsSummaries& other) +{ + Total.AggregateFrom(other.Total); + MinOpts.AggregateFrom(other.MinOpts); + Opts.AggregateFrom(other.Opts); +} struct FileHandleWrapper { @@ -31,7 +41,23 @@ struct FileHandleWrapper HANDLE hFile; }; -bool MetricsSummary::SaveToFile(const char* path) +static bool FilePrintf(HANDLE hFile, const char* fmt, ...) +{ + va_list args; + va_start(args, fmt); + + char buffer[4096]; + int len = vsprintf_s(buffer, fmt, args); + DWORD numWritten; + bool result = + WriteFile(hFile, buffer, static_cast(len), &numWritten, nullptr) && (numWritten == static_cast(len)); + + va_end(args); + + return result; +} + +bool MetricsSummaries::SaveToFile(const char* path) { FileHandleWrapper file(CreateFile(path, GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); if (file.get() == INVALID_HANDLE_VALUE) @@ -39,29 +65,37 @@ bool MetricsSummary::SaveToFile(const char* path) return false; } - char buffer[4096]; - int len = - sprintf_s( - buffer, sizeof(buffer), - "Successful compiles,Failing compiles,Missing compiles,Code bytes,Diffed code bytes,Executed instructions,Diff executed instructions\n" - "%d,%d,%d,%lld,%lld,%lld,%lld\n", - SuccessfulCompiles, - FailingCompiles, - MissingCompiles, - NumCodeBytes, - NumDiffedCodeBytes, - NumExecutedInstructions, - NumDiffExecutedInstructions); - DWORD numWritten; - if (!WriteFile(file.get(), buffer, static_cast(len), &numWritten, nullptr) || numWritten != static_cast(len)) + if (!FilePrintf( + file.get(), + "Successful compiles,Failing compiles,Missing compiles," + "Code bytes,Diffed code bytes,Executed instructions,Diff executed instructions,Name\n")) { return false; } - return true; + return + WriteRow(file.get(), "Total", Total) && + WriteRow(file.get(), "MinOpts", MinOpts) && + WriteRow(file.get(), "Opts", Opts); +} + +bool MetricsSummaries::WriteRow(HANDLE hFile, const char* name, const MetricsSummary& summary) +{ + return + FilePrintf( + hFile, + "%d,%d,%d,%lld,%lld,%lld,%lld,%s\n", + summary.SuccessfulCompiles, + summary.FailingCompiles, + summary.MissingCompiles, + summary.NumCodeBytes, + summary.NumDiffedCodeBytes, + summary.NumExecutedInstructions, + summary.NumDiffExecutedInstructions, + name); } -bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* metrics) +bool MetricsSummaries::LoadFromFile(const char* path, MetricsSummaries* metrics) { FileHandleWrapper file(CreateFile(path, GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)); if (file.get() == INVALID_HANDLE_VALUE) @@ -76,38 +110,73 @@ bool MetricsSummary::LoadFromFile(const char* path, MetricsSummary* metrics) } DWORD stringLen = static_cast(len.QuadPart); - std::vector content(stringLen + 1); + std::vector content(stringLen); DWORD numRead; - if (!ReadFile(file.get(), content.data(), stringLen, &numRead, nullptr) || numRead != stringLen) + if (!ReadFile(file.get(), content.data(), stringLen, &numRead, nullptr) || (numRead != stringLen)) { return false; } - content[stringLen] = '\0'; - - int scanResult = - sscanf_s( - content.data(), - "Successful compiles,Failing compiles,Missing compiles,Code bytes,Diffed code bytes,Executed instructions,Diff executed instructions\n" - "%d,%d,%d,%lld,%lld,%lld,%lld\n", - &metrics->SuccessfulCompiles, - &metrics->FailingCompiles, - &metrics->MissingCompiles, - &metrics->NumCodeBytes, - &metrics->NumDiffedCodeBytes, - &metrics->NumExecutedInstructions, - &metrics->NumDiffExecutedInstructions); - - return scanResult == 7; -} + std::vector line; + size_t index = 0; + auto nextLine = [&line, &content, &index]() + { + size_t end = index; + while ((end < content.size()) && (content[end] != '\r') && (content[end] != '\n')) + { + end++; + } -void MetricsSummary::AggregateFrom(const MetricsSummary& other) -{ - SuccessfulCompiles += other.SuccessfulCompiles; - FailingCompiles += other.FailingCompiles; - MissingCompiles += other.MissingCompiles; - NumCodeBytes += other.NumCodeBytes; - NumDiffedCodeBytes += other.NumDiffedCodeBytes; - NumExecutedInstructions += other.NumExecutedInstructions; - NumDiffExecutedInstructions += other.NumDiffExecutedInstructions; + line.resize(end - index + 1); + memcpy(line.data(), &content[index], end - index); + line[end - index] = '\0'; + + index = end; + if ((index < content.size()) && (content[index] == '\r')) + index++; + if ((index < content.size()) && (content[index] == '\n')) + index++; + }; + + *metrics = MetricsSummaries(); + nextLine(); + bool result = true; + while (index < content.size()) + { + nextLine(); + MetricsSummary summary; + + char name[32]; + int scanResult = + sscanf_s( + line.data(), + "%d,%d,%d,%lld,%lld,%lld,%lld,%s", + &summary.SuccessfulCompiles, + &summary.FailingCompiles, + &summary.MissingCompiles, + &summary.NumCodeBytes, + &summary.NumDiffedCodeBytes, + &summary.NumExecutedInstructions, + &summary.NumDiffExecutedInstructions, + name, (unsigned)sizeof(name)); + + if (scanResult == 8) + { + MetricsSummary* tarSummary = nullptr; + if (strcmp(name, "Total") == 0) + metrics->Total = summary; + else if (strcmp(name, "MinOpts") == 0) + metrics->MinOpts = summary; + else if (strcmp(name, "Opts") == 0) + metrics->Opts = summary; + else + result = false; + } + else + { + result = false; + } + } + + return result; } diff --git a/src/coreclr/tools/superpmi/superpmi/metricssummary.h b/src/coreclr/tools/superpmi/superpmi/metricssummary.h index 51208355755208..03bff16a94db68 100644 --- a/src/coreclr/tools/superpmi/superpmi/metricssummary.h +++ b/src/coreclr/tools/superpmi/superpmi/metricssummary.h @@ -4,9 +4,8 @@ #ifndef _MetricsSummary #define _MetricsSummary -class MetricsSummary +struct MetricsSummary { -public: // Number of methods successfully jitted. int SuccessfulCompiles = 0; // Number of methods that failed jitting. @@ -23,9 +22,22 @@ class MetricsSummary // Number of executed instructions inside contexts that were successfully diffed. long long NumDiffExecutedInstructions = 0; - bool SaveToFile(const char* path); - static bool LoadFromFile(const char* path, MetricsSummary* metrics); void AggregateFrom(const MetricsSummary& other); }; +class MetricsSummaries +{ +public: + MetricsSummary Total; + MetricsSummary MinOpts; + MetricsSummary Opts; + + void AggregateFrom(const MetricsSummaries& other); + + bool SaveToFile(const char* path); + static bool LoadFromFile(const char* path, MetricsSummaries* metrics); +private: + static bool WriteRow(HANDLE hFile, const char* name, const MetricsSummary& summary); +}; + #endif diff --git a/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp b/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp index 92b172e196d1c0..dfca5adcc376a6 100644 --- a/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/parallelsuperpmi.cpp @@ -294,12 +294,16 @@ void ProcessChildStdOut(const CommandLine::Options& o, } } -static bool ProcessChildMetrics(const char* baseMetricsSummaryPath, const char* diffMetricsSummaryPath, MetricsSummary* baseMetrics, MetricsSummary* diffMetrics) +static bool ProcessChildMetrics( + const char* baseMetricsSummaryPath, + MetricsSummaries* baseMetrics, + const char* diffMetricsSummaryPath, + MetricsSummaries* diffMetrics) { if (baseMetricsSummaryPath != nullptr) { - MetricsSummary childBaseMetrics; - if (!MetricsSummary::LoadFromFile(baseMetricsSummaryPath, &childBaseMetrics)) + MetricsSummaries childBaseMetrics; + if (!MetricsSummaries::LoadFromFile(baseMetricsSummaryPath, &childBaseMetrics)) { LogError("Couldn't load base metrics summary created by child process"); return false; @@ -310,8 +314,8 @@ static bool ProcessChildMetrics(const char* baseMetricsSummaryPath, const char* if (diffMetricsSummaryPath != nullptr) { - MetricsSummary childDiffMetrics; - if (!MetricsSummary::LoadFromFile(diffMetricsSummaryPath, &childDiffMetrics)) + MetricsSummaries childDiffMetrics; + if (!MetricsSummaries::LoadFromFile(diffMetricsSummaryPath, &childDiffMetrics)) { LogError("Couldn't load diff metrics summary created by child process"); return false; @@ -556,13 +560,13 @@ int doParallelSuperPMI(CommandLine::Options& o) if (o.baseMetricsSummaryFile != nullptr) { wd.baseMetricsSummaryPath = new char[MAX_PATH]; - sprintf_s(wd.baseMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-BaseMetricsSummary-%u-%d.txt", tempPath, randNumber, i); + sprintf_s(wd.baseMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-BaseMetricsSummary-%u-%d.csv", tempPath, randNumber, i); } if (o.diffMetricsSummaryFile != nullptr) { wd.diffMetricsSummaryPath = new char[MAX_PATH]; - sprintf_s(wd.diffMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-DiffMetricsSummary-%u-%d.txt", tempPath, randNumber, i); + sprintf_s(wd.diffMetricsSummaryPath, MAX_PATH, "%sParallelSuperPMI-DiffMetricsSummary-%u-%d.csv", tempPath, randNumber, i); } wd.stdOutputPath = new char[MAX_PATH]; @@ -693,8 +697,8 @@ 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; + MetricsSummaries baseMetrics; + MetricsSummaries 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 @@ -703,7 +707,7 @@ int doParallelSuperPMI(CommandLine::Options& o) 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); + ProcessChildMetrics(wd.baseMetricsSummaryPath, &baseMetrics, wd.diffMetricsSummaryPath, &diffMetrics); if (usageError) break; diff --git a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp index c43ed6fad684d2..9740ffc3ed73ca 100644 --- a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp @@ -269,8 +269,8 @@ int __cdecl main(int argc, char* argv[]) } } - MetricsSummary totalBaseMetrics; - MetricsSummary totalDiffMetrics; + MetricsSummaries totalBaseMetrics; + MetricsSummaries totalDiffMetrics; while (true) { @@ -367,18 +367,32 @@ int __cdecl main(int argc, char* argv[]) } MetricsSummary baseMetrics; + CORJIT_FLAGS jitFlags; jittedCount++; st3.Start(); - res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &baseMetrics); + res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &baseMetrics, &jitFlags); st3.Stop(); LogDebug("Method %d compiled%s in %fms, result %d", reader->GetMethodContextIndex(), (o.nameOfJit2 == nullptr) ? "" : " by JIT1", st3.GetMilliseconds(), res); - totalBaseMetrics.AggregateFrom(baseMetrics); + bool isMinOpts = + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE) || + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT) || + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); - if ((res == JitInstance::RESULT_SUCCESS) && Logger::IsLogLevelEnabled(LOGLEVEL_DEBUG)) + MetricsSummary& totalBaseMetricsOpts = isMinOpts ? totalBaseMetrics.MinOpts : totalBaseMetrics.Opts; + MetricsSummary& totalDiffMetricsOpts = isMinOpts ? totalDiffMetrics.MinOpts : totalDiffMetrics.Opts; + + totalBaseMetrics.Total.AggregateFrom(baseMetrics); + + if (res == JitInstance::RESULT_SUCCESS) { - mc->cr->dumpToConsole(); // Dump the compile results if doing debug logging + totalBaseMetricsOpts.AggregateFrom(baseMetrics); + + if (Logger::IsLogLevelEnabled(LOGLEVEL_DEBUG)) + { + mc->cr->dumpToConsole(); // Dump the compile results if doing debug logging + } } MetricsSummary diffMetrics; @@ -392,16 +406,21 @@ int __cdecl main(int argc, char* argv[]) mc->cr = new CompileResult(); st4.Start(); - res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &diffMetrics); + res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &diffMetrics, &jitFlags); st4.Stop(); LogDebug("Method %d compiled by JIT2 in %fms, result %d", reader->GetMethodContextIndex(), st4.GetMilliseconds(), res2); - totalDiffMetrics.AggregateFrom(diffMetrics); + totalDiffMetrics.Total.AggregateFrom(diffMetrics); - if ((res2 == JitInstance::RESULT_SUCCESS) && Logger::IsLogLevelEnabled(LOGLEVEL_DEBUG)) + if (res2 == JitInstance::RESULT_SUCCESS) { - mc->cr->dumpToConsole(); // Dump the compile results if doing debug logging + totalDiffMetricsOpts.AggregateFrom(diffMetrics); + + if (Logger::IsLogLevelEnabled(LOGLEVEL_DEBUG)) + { + mc->cr->dumpToConsole(); // Dump the compile results if doing debug logging + } } if (res2 == JitInstance::RESULT_ERROR) @@ -542,11 +561,17 @@ int __cdecl main(int argc, char* argv[]) { InvokeNearDiffer(&nearDiffer, &o, &mc, &crl, &matchCount, &reader, &failingToReplayMCL, &diffMCL); - totalBaseMetrics.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; - totalDiffMetrics.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; + totalBaseMetrics.Total.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; + totalDiffMetrics.Total.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; + + totalBaseMetricsOpts.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; + totalDiffMetricsOpts.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; + + totalBaseMetrics.Total.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; + totalDiffMetrics.Total.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; - totalBaseMetrics.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; - totalDiffMetrics.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; + totalBaseMetricsOpts.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; + totalDiffMetricsOpts.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; } } } From 4ad92a1253f124d6775498530ebd9a8d16a9e907 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 25 Aug 2022 17:40:32 +0200 Subject: [PATCH 03/22] Fix PAL build PAL does not have the template variant of vsprintf_s. --- src/coreclr/tools/superpmi/superpmi/metricssummary.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp index 64d9c5216c108c..822c47fe1d1b05 100644 --- a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp +++ b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp @@ -47,7 +47,7 @@ static bool FilePrintf(HANDLE hFile, const char* fmt, ...) va_start(args, fmt); char buffer[4096]; - int len = vsprintf_s(buffer, fmt, args); + int len = vsprintf_s(buffer, ARRAYSIZE(buffer), fmt, args); DWORD numWritten; bool result = WriteFile(hFile, buffer, static_cast(len), &numWritten, nullptr) && (numWritten == static_cast(len)); From 13a4ab7cd30ee0d8b68244e3f83e4546e2da0f86 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 25 Aug 2022 21:46:37 +0200 Subject: [PATCH 04/22] Get some diffs in release --- src/coreclr/jit/forwardsub.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index 6ee56015106ea7..c9e68c6ab784c6 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -114,16 +114,14 @@ PhaseStatus Compiler::fgForwardSub() { return PhaseStatus::MODIFIED_NOTHING; } - else +#endif + + CLRRandom rng; + rng.Init(info.compILCodeSize ^ info.compILlocalsCount ^ 0x12345678); + if (rng.Next(10) == 0) { - CLRRandom rng; - rng.Init(info.compMethodHash() ^ 0x12345678); - if (rng.Next(10) == 0) - { - return PhaseStatus::MODIFIED_NOTHING; - } + return PhaseStatus::MODIFIED_NOTHING; } -#endif bool changed = false; From 75f71a0d23671175e12ac261977d53c9289e6db2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 25 Aug 2022 22:00:33 +0200 Subject: [PATCH 05/22] Remove percentage, try some color --- src/coreclr/scripts/superpmi.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index cf15911a984abb..780170c096e307 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1838,7 +1838,9 @@ def format_delta(base, diff): if base > 0: pct = " ({}{:.2f}%)".format(plus_if_positive, (diff - base) / base * 100) - return "{}{:,d} B{}".format(plus_if_positive, diff - base, pct) + #return "{}{:,d} B{}".format(plus_if_positive, diff - base, pct) + color = "red" if diff > base else "green" + return "{}{:,d}".format(color, plus_if_positive, diff - base) diffed_contexts = sum(int(diff_metrics["Total"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) @@ -1846,14 +1848,16 @@ def format_delta(base, diff): missing_base_contexts = sum(int(base_metrics["Total"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) missing_diff_contexts = sum(int(diff_metrics["Total"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n".format( + write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n".format( diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) - write_fh.write("{:,d} contexts missed with the base JIT and {:,d} missed with the diff JIT.\n\n".format(missing_base_contexts, missing_diff_contexts)) + color_base = "orange" if missing_base_contexts > 0 else "green" + color_diff = "orange" if missing_diff_contexts > 0 else "green" + write_fh.write("{:,d} contexts missed with the base JIT and {:,d} missed with the diff JIT.\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): # First write an overview table - write_fh.write("|Collection|Overall|MinOpts|Opts|\n") + write_fh.write("|Collection|Overall (bytes)|MinOpts (bytes)|Opts (bytes)|\n") write_fh.write("|---|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: if not has_diffs: @@ -2117,7 +2121,8 @@ def check(col): return check("Total") or check("MinOpts") or check("Opts") def format_pct(base_instructions, diff_instructions): plus_if_positive = "+" if diff_instructions > base_instructions else "" - return "{}{:.2f}%".format(plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) + color = "red" if diff_instructions > base_instructions else "green" + return "{}{:.2f}%".format(color, plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) if any(is_significant(base, diff) for (_, base, diff) in tp_diffs): write_fh.write("|Collection|Overall|MinOpts|Opts|\n") From 631d489da19e73ad10cc1d02da7df2b0191cf5c8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 29 Aug 2022 14:02:02 +0200 Subject: [PATCH 06/22] Various adjustments * Rename Total -> Overall, Opts -> FullOpts * Move Overall column to the right * Adjust some colors * Include minipal/utils.h so we can use ARRAY_SIZE, which hopefully fixes the build on non-Windows. --- src/coreclr/scripts/superpmi.py | 68 +++++++++---------- .../superpmi/superpmi-shared/standardpch.h | 2 + .../superpmi/superpmi/metricssummary.cpp | 18 ++--- .../tools/superpmi/superpmi/metricssummary.h | 4 +- .../tools/superpmi/superpmi/superpmi.cpp | 16 ++--- 5 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 780170c096e307..180475de653be7 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -484,7 +484,7 @@ def read_csv_metrics(path): path (str) : path to .csv file Returns: - A dictionary of dictionaries. For example, dict["Total"]["Successful + A dictionary of dictionaries. For example, dict["Overall"]["Successful compiles"] will access the total number of successful compiles and dict["MinOpts"]["Successful compiles"] will access the number of minopts compilations. @@ -1192,7 +1192,7 @@ def print_superpmi_result(return_code, coreclr_args, base_metrics, diff_metrics) those return codes. """ if return_code == 0: - logging.info("Clean SuperPMI {} ({} contexts processed)".format("replay" if diff_metrics is None else "diff", base_metrics["Total"]["Successful compiles"])) + logging.info("Clean SuperPMI {} ({} contexts processed)".format("replay" if diff_metrics is None else "diff", base_metrics["Overall"]["Successful compiles"])) elif return_code == -1 or return_code == 4294967295: logging.error("General fatal error") elif return_code == -2 or return_code == 4294967294: @@ -1202,13 +1202,13 @@ def print_superpmi_result(return_code, coreclr_args, base_metrics, diff_metrics) elif return_code == 2: logging.warning("Asm diffs found") elif return_code == 3: - missing_base = int(base_metrics["Total"]["Missing compiles"]) - total_contexts = int(base_metrics["Total"]["Successful compiles"]) + int(base_metrics["Total"]["Failing compiles"]) + missing_base = int(base_metrics["Overall"]["Missing compiles"]) + total_contexts = int(base_metrics["Overall"]["Successful compiles"]) + int(base_metrics["Overall"]["Failing compiles"]) if diff_metrics is None: logging.warning("SuperPMI encountered missing data for {} out of {} contexts".format(missing_base, total_contexts)) else: - missing_diff = int(diff_metrics["Total"]["Missing compiles"]) + missing_diff = int(diff_metrics["Overall"]["Missing compiles"]) logging.warning("SuperPMI encountered missing data. Missing with base JIT: {}. Missing with diff JIT: {}. Total contexts: {}.".format(missing_base, missing_diff, total_contexts)) elif return_code == 139 and coreclr_args.host_os != "windows": @@ -1718,8 +1718,8 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: logging.debug("") if base_metrics is not None and diff_metrics is not None: - base_bytes = int(base_metrics["Total"]["Diffed code bytes"]) - diff_bytes = int(diff_metrics["Total"]["Diffed code bytes"]) + base_bytes = int(base_metrics["Overall"]["Diffed code bytes"]) + diff_bytes = int(diff_metrics["Overall"]["Diffed code bytes"]) logging.info("Total bytes of base: {}".format(base_bytes)) logging.info("Total bytes of diff: {}".format(diff_bytes)) delta_bytes = diff_bytes - base_bytes @@ -1790,9 +1790,9 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: logging.warning("No textual differences found in generated JitDump. Is this an issue with coredistools?") if base_metrics is not None and diff_metrics is not None: - missing_base = int(base_metrics["Total"]["Missing compiles"]) - missing_diff = int(diff_metrics["Total"]["Missing compiles"]) - total_contexts = int(base_metrics["Total"]["Successful compiles"]) + int(base_metrics["Total"]["Failing compiles"]) + missing_base = int(base_metrics["Overall"]["Missing compiles"]) + missing_diff = int(diff_metrics["Overall"]["Missing compiles"]) + total_contexts = int(base_metrics["Overall"]["Successful compiles"]) + int(base_metrics["Overall"]["Failing compiles"]) if missing_base > 0 or missing_diff > 0: logging.warning("Warning: SuperPMI encountered missing data during the diff. The diff summary printed above may be misleading.") @@ -1842,22 +1842,22 @@ def format_delta(base, diff): color = "red" if diff > base else "green" return "{}{:,d}".format(color, plus_if_positive, diff - base) - diffed_contexts = sum(int(diff_metrics["Total"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + diffed_contexts = sum(int(diff_metrics["Overall"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - diffed_opts_contexts = sum(int(diff_metrics["Opts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - missing_base_contexts = sum(int(base_metrics["Total"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) - missing_diff_contexts = sum(int(diff_metrics["Total"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + diffed_opts_contexts = sum(int(diff_metrics["FullOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + missing_base_contexts = sum(int(base_metrics["Overall"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) + missing_diff_contexts = sum(int(diff_metrics["Overall"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n".format( + write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n\n".format( diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) - color_base = "orange" if missing_base_contexts > 0 else "green" - color_diff = "orange" if missing_diff_contexts > 0 else "green" - write_fh.write("{:,d} contexts missed with the base JIT and {:,d} missed with the diff JIT.\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) + color_base = "#d35400" if missing_base_contexts > 0 else "green" + color_diff = "#d35400" if missing_diff_contexts > 0 else "green" + write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): # First write an overview table - write_fh.write("|Collection|Overall (bytes)|MinOpts (bytes)|Opts (bytes)|\n") + write_fh.write("|Collection|MinOpts (bytes)|Opts (bytes)|Overall (bytes)|\n") write_fh.write("|---|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: if not has_diffs: @@ -1865,15 +1865,15 @@ def format_delta(base, diff): write_fh.write("|{}|{}|{}|{}|\n".format( mch_file, - format_delta( - int(base_metrics["Total"]["Diffed code bytes"]), - int(diff_metrics["Total"]["Diffed code bytes"])), format_delta( int(base_metrics["MinOpts"]["Diffed code bytes"]), int(diff_metrics["MinOpts"]["Diffed code bytes"])), format_delta( - int(base_metrics["Opts"]["Diffed code bytes"]), - int(diff_metrics["Opts"]["Diffed code bytes"])))) + int(base_metrics["FullOpts"]["Diffed code bytes"]), + int(diff_metrics["FullOpts"]["Diffed code bytes"])), + format_delta( + int(base_metrics["Overall"]["Diffed code bytes"]), + int(diff_metrics["Overall"]["Diffed code bytes"])))) write_fh.write("\n") else: @@ -1888,11 +1888,11 @@ def format_delta(base, diff): for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: write_fh.write("|{}|{:,d}|{:,d}|{:,d}|{:,d}|{:,d}|\n".format( mch_file, - int(diff_metrics["Total"]["Successful compiles"]), - int(base_metrics["Total"]["Missing compiles"]), - int(diff_metrics["Total"]["Missing compiles"]), + int(diff_metrics["Overall"]["Successful compiles"]), + int(base_metrics["Overall"]["Missing compiles"]), + int(diff_metrics["Overall"]["Missing compiles"]), int(diff_metrics["MinOpts"]["Successful compiles"]), - int(diff_metrics["Opts"]["Successful compiles"]))) + int(diff_metrics["FullOpts"]["Successful compiles"]))) write_fh.write("\n") @@ -2065,8 +2065,8 @@ def replay_with_throughput_diff(self): diff_metrics = read_csv_metrics(diff_metrics_summary_file) if base_metrics is not None and diff_metrics is not None: - base_instructions = int(base_metrics["Total"]["Diff executed instructions"]) - diff_instructions = int(diff_metrics["Total"]["Diff executed instructions"]) + base_instructions = int(base_metrics["Overall"]["Diff executed instructions"]) + diff_instructions = int(diff_metrics["Overall"]["Diff executed instructions"]) logging.info("Total instructions executed by base: {}".format(base_instructions)) logging.info("Total instructions executed by diff: {}".format(diff_instructions)) @@ -2118,7 +2118,7 @@ def is_significant(base, diff): def check(col): return is_significant_pct(int(base[col]["Diff executed instructions"]), int(diff[col]["Diff executed instructions"])) - return check("Total") or check("MinOpts") or check("Opts") + return check("Overall") or check("MinOpts") or check("FullOpts") def format_pct(base_instructions, diff_instructions): plus_if_positive = "+" if diff_instructions > base_instructions else "" color = "red" if diff_instructions > base_instructions else "green" @@ -2136,15 +2136,15 @@ def format_pct_for_col(col): if is_significant(base, diff): write_fh.write("|{}|{}|{}|{}|\n".format( mch_file, - format_pct_for_col("Total"), format_pct_for_col("MinOpts"), - format_pct_for_col("Opts"))) + format_pct_for_col("FullOpts"), + format_pct_for_col("Overall"))) else: write_fh.write("No significant throughput differences found\n") write_fh.write("\n
\n") write_fh.write("Details\n\n") - for (disp, col) in [("All", "Total"), ("MinOpts", "MinOpts"), ("Optimized", "Opts")]: + for (disp, col) in [("All", "Overall"), ("MinOpts", "MinOpts"), ("Optimized", "FullOpts")]: write_fh.write("{} contexts:\n\n".format(disp)) write_fh.write("|Collection|Base # instructions|Diff # instructions|PDIFF|\n") write_fh.write("|---|---|---|---|\n") diff --git a/src/coreclr/tools/superpmi/superpmi-shared/standardpch.h b/src/coreclr/tools/superpmi/superpmi-shared/standardpch.h index af32198b1b73e2..dc4ce235ffbf2a 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/standardpch.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/standardpch.h @@ -116,4 +116,6 @@ static inline void __debugbreak() } #endif +#include + #endif // STANDARDPCH_H diff --git a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp index 822c47fe1d1b05..9ab5b598f93485 100644 --- a/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp +++ b/src/coreclr/tools/superpmi/superpmi/metricssummary.cpp @@ -18,9 +18,9 @@ void MetricsSummary::AggregateFrom(const MetricsSummary& other) void MetricsSummaries::AggregateFrom(const MetricsSummaries& other) { - Total.AggregateFrom(other.Total); + Overall.AggregateFrom(other.Overall); MinOpts.AggregateFrom(other.MinOpts); - Opts.AggregateFrom(other.Opts); + FullOpts.AggregateFrom(other.FullOpts); } struct FileHandleWrapper @@ -47,7 +47,7 @@ static bool FilePrintf(HANDLE hFile, const char* fmt, ...) va_start(args, fmt); char buffer[4096]; - int len = vsprintf_s(buffer, ARRAYSIZE(buffer), fmt, args); + int len = vsprintf_s(buffer, ARRAY_SIZE(buffer), fmt, args); DWORD numWritten; bool result = WriteFile(hFile, buffer, static_cast(len), &numWritten, nullptr) && (numWritten == static_cast(len)); @@ -74,9 +74,9 @@ bool MetricsSummaries::SaveToFile(const char* path) } return - WriteRow(file.get(), "Total", Total) && + WriteRow(file.get(), "Overall", Overall) && WriteRow(file.get(), "MinOpts", MinOpts) && - WriteRow(file.get(), "Opts", Opts); + WriteRow(file.get(), "FullOpts", FullOpts); } bool MetricsSummaries::WriteRow(HANDLE hFile, const char* name, const MetricsSummary& summary) @@ -163,12 +163,12 @@ bool MetricsSummaries::LoadFromFile(const char* path, MetricsSummaries* metrics) if (scanResult == 8) { MetricsSummary* tarSummary = nullptr; - if (strcmp(name, "Total") == 0) - metrics->Total = summary; + if (strcmp(name, "Overall") == 0) + metrics->Overall = summary; else if (strcmp(name, "MinOpts") == 0) metrics->MinOpts = summary; - else if (strcmp(name, "Opts") == 0) - metrics->Opts = summary; + else if (strcmp(name, "FullOpts") == 0) + metrics->FullOpts = summary; else result = false; } diff --git a/src/coreclr/tools/superpmi/superpmi/metricssummary.h b/src/coreclr/tools/superpmi/superpmi/metricssummary.h index 03bff16a94db68..1f1ccc47cdaa67 100644 --- a/src/coreclr/tools/superpmi/superpmi/metricssummary.h +++ b/src/coreclr/tools/superpmi/superpmi/metricssummary.h @@ -28,9 +28,9 @@ struct MetricsSummary class MetricsSummaries { public: - MetricsSummary Total; + MetricsSummary Overall; MetricsSummary MinOpts; - MetricsSummary Opts; + MetricsSummary FullOpts; void AggregateFrom(const MetricsSummaries& other); diff --git a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp index 9740ffc3ed73ca..1009535c1c7fc3 100644 --- a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp @@ -380,10 +380,10 @@ int __cdecl main(int argc, char* argv[]) jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT) || jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); - MetricsSummary& totalBaseMetricsOpts = isMinOpts ? totalBaseMetrics.MinOpts : totalBaseMetrics.Opts; - MetricsSummary& totalDiffMetricsOpts = isMinOpts ? totalDiffMetrics.MinOpts : totalDiffMetrics.Opts; + MetricsSummary& totalBaseMetricsOpts = isMinOpts ? totalBaseMetrics.MinOpts : totalBaseMetrics.FullOpts; + MetricsSummary& totalDiffMetricsOpts = isMinOpts ? totalDiffMetrics.MinOpts : totalDiffMetrics.FullOpts; - totalBaseMetrics.Total.AggregateFrom(baseMetrics); + totalBaseMetrics.Overall.AggregateFrom(baseMetrics); if (res == JitInstance::RESULT_SUCCESS) { @@ -411,7 +411,7 @@ int __cdecl main(int argc, char* argv[]) LogDebug("Method %d compiled by JIT2 in %fms, result %d", reader->GetMethodContextIndex(), st4.GetMilliseconds(), res2); - totalDiffMetrics.Total.AggregateFrom(diffMetrics); + totalDiffMetrics.Overall.AggregateFrom(diffMetrics); if (res2 == JitInstance::RESULT_SUCCESS) { @@ -561,14 +561,14 @@ int __cdecl main(int argc, char* argv[]) { InvokeNearDiffer(&nearDiffer, &o, &mc, &crl, &matchCount, &reader, &failingToReplayMCL, &diffMCL); - totalBaseMetrics.Total.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; - totalDiffMetrics.Total.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; + totalBaseMetrics.Overall.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; + totalDiffMetrics.Overall.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; totalBaseMetricsOpts.NumDiffedCodeBytes += baseMetrics.NumCodeBytes; totalDiffMetricsOpts.NumDiffedCodeBytes += diffMetrics.NumCodeBytes; - totalBaseMetrics.Total.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; - totalDiffMetrics.Total.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; + totalBaseMetrics.Overall.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; + totalDiffMetrics.Overall.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; totalBaseMetricsOpts.NumDiffExecutedInstructions += baseMetrics.NumExecutedInstructions; totalDiffMetricsOpts.NumDiffExecutedInstructions += diffMetrics.NumExecutedInstructions; From c8d4b8e21aeab91534d479ca205708c7bdcdb179 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 2 Sep 2022 15:04:40 +0200 Subject: [PATCH 07/22] Reorder/rename some columns --- src/coreclr/scripts/superpmi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 180475de653be7..87490baa9f381f 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1857,7 +1857,7 @@ def format_delta(base, diff): if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): # First write an overview table - write_fh.write("|Collection|MinOpts (bytes)|Opts (bytes)|Overall (bytes)|\n") + write_fh.write("|Collection|MinOpts (bytes)|FullOpts (bytes)|Overall (bytes)|\n") write_fh.write("|---|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: if not has_diffs: @@ -2125,7 +2125,7 @@ def format_pct(base_instructions, diff_instructions): return "{}{:.2f}%".format(color, plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) if any(is_significant(base, diff) for (_, base, diff) in tp_diffs): - write_fh.write("|Collection|Overall|MinOpts|Opts|\n") + write_fh.write("|Collection|MinOpts|FullOpts|Overall|\n") write_fh.write("|---|---|---|---|\n") for mch_file, base, diff in tp_diffs: def format_pct_for_col(col): From 5d79fb92a2b6f7ca693d449d90559cd32f752cc6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 5 Sep 2022 14:14:58 +0200 Subject: [PATCH 08/22] Separate sections for overall/minopts/fullopts --- src/coreclr/scripts/superpmi.py | 86 ++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 34 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 180475de653be7..3b5be902af6036 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1834,11 +1834,6 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: with open(overall_md_summary_file, "w") as write_fh: def format_delta(base, diff): plus_if_positive = "+" if diff >= base else "" - pct = "" - if base > 0: - pct = " ({}{:.2f}%)".format(plus_if_positive, (diff - base) / base * 100) - - #return "{}{:,d} B{}".format(plus_if_positive, diff - base, pct) color = "red" if diff > base else "green" return "{}{:,d}".format(color, plus_if_positive, diff - base) @@ -1856,26 +1851,35 @@ def format_delta(base, diff): write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): - # First write an overview table - write_fh.write("|Collection|MinOpts (bytes)|Opts (bytes)|Overall (bytes)|\n") - write_fh.write("|---|---|---|---|\n") - for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: - if not has_diffs: - continue + def write_table(col): + write_fh.write("|Collection|Base size (KiB)|Diff size (bytes)|\n") + write_fh.write("|---|---|---|\n") + for (mch_file, base_metrics, diff_metrics, has_diffs, _) in asm_diffs: + if not has_diffs: + continue + + write_fh.write("|{}|{:,.0f}|{}|\n".format( + mch_file, + int(base_metrics[col]["Diffed code bytes"]) / 1024, + format_delta( + int(base_metrics[col]["Diffed code bytes"]), + int(diff_metrics[col]["Diffed code bytes"])))) - write_fh.write("|{}|{}|{}|{}|\n".format( - mch_file, - format_delta( - int(base_metrics["MinOpts"]["Diffed code bytes"]), - int(diff_metrics["MinOpts"]["Diffed code bytes"])), - format_delta( - int(base_metrics["FullOpts"]["Diffed code bytes"]), - int(diff_metrics["FullOpts"]["Diffed code bytes"])), - format_delta( - int(base_metrics["Overall"]["Diffed code bytes"]), - int(diff_metrics["Overall"]["Diffed code bytes"])))) + write_fh.write("\n") - write_fh.write("\n") + write_table("Overall") + + def write_pivot_section(col): + write_fh.write("\n
\n") + sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) + sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) + + write_fh.write("{} ({} bytes)\n\n".format(col, format_delta(sum_base, sum_diff))) + write_table(col) + write_fh.write("\n
\n") + + write_pivot_section("MinOpts") + write_pivot_section("FullOpts") else: write_fh.write("No diffs found.\n") @@ -2125,20 +2129,34 @@ def format_pct(base_instructions, diff_instructions): return "{}{:.2f}%".format(color, plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) if any(is_significant(base, diff) for (_, base, diff) in tp_diffs): - write_fh.write("|Collection|Overall|MinOpts|Opts|\n") - write_fh.write("|---|---|---|---|\n") - for mch_file, base, diff in tp_diffs: - def format_pct_for_col(col): + def write_table(col): + write_fh.write("|Collection|PDIFF|\n") + write_fh.write("|---|---|\n") + for mch_file, base, diff in tp_diffs: base_instructions = int(base[col]["Diff executed instructions"]) diff_instructions = int(diff[col]["Diff executed instructions"]) - return format_pct(base_instructions, diff_instructions) - if is_significant(base, diff): - write_fh.write("|{}|{}|{}|{}|\n".format( - mch_file, - format_pct_for_col("MinOpts"), - format_pct_for_col("FullOpts"), - format_pct_for_col("Overall"))) + if is_significant_pct(base_instructions, diff_instructions): + write_fh.write("|{}|{}|\n".format( + mch_file, + format_pct(base_instructions, diff_instructions))) + + write_fh.write("\n") + + write_table("Overall") + + def write_pivot_section(col): + write_fh.write("\n
\n") + sum_base = sum(int(base_metrics[col]["Diff executed instructions"]) for (_, base_metrics, _) in tp_diffs) + sum_diff = sum(int(diff_metrics[col]["Diff executed instructions"]) for (_, _, diff_metrics) in tp_diffs) + + write_fh.write("{} ({})\n\n".format(col, format_pct(sum_base, sum_diff)))) + write_table(col) + write_fh.write("\n
\n") + + write_pivot_section("MinOpts") + write_pivot_section("FullOpts") + else: write_fh.write("No significant throughput differences found\n") From e7087df028ae12a78740c5fa64edd3cee0abd498 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 5 Sep 2022 15:19:45 +0200 Subject: [PATCH 09/22] Fixes --- src/coreclr/scripts/superpmi.py | 64 +++++++++++++++------------------ 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 69f7f46839fd84..1054895d6a1114 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1835,7 +1835,7 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: def format_delta(base, diff): plus_if_positive = "+" if diff >= base else "" color = "red" if diff > base else "green" - return "{}{:,d}".format(color, plus_if_positive, diff - base) + return "{}{:,.3f}".format(color, plus_if_positive, (diff - base) / 1000) diffed_contexts = sum(int(diff_metrics["Overall"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) @@ -1851,8 +1851,13 @@ def format_delta(base, diff): write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): - def write_table(col): - write_fh.write("|Collection|Base size (KiB)|Diff size (bytes)|\n") + def write_pivot_section(col, open): + write_fh.write("\n\n".format(" open" if open else "")) + sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) + sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) + + write_fh.write("{} ({} KB)\n\n".format(col, format_delta(sum_base, sum_diff))) + write_fh.write("|Collection|Base size (KB)|Diff size (KB)|\n") write_fh.write("|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, _) in asm_diffs: if not has_diffs: @@ -1860,26 +1865,16 @@ def write_table(col): write_fh.write("|{}|{:,.0f}|{}|\n".format( mch_file, - int(base_metrics[col]["Diffed code bytes"]) / 1024, + int(base_metrics[col]["Diffed code bytes"]), format_delta( int(base_metrics[col]["Diffed code bytes"]), int(diff_metrics[col]["Diffed code bytes"])))) - write_fh.write("\n") + write_fh.write("\n\n
\n") - write_table("Overall") - - def write_pivot_section(col): - write_fh.write("\n
\n") - sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) - sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) - - write_fh.write("{} ({} bytes)\n\n".format(col, format_delta(sum_base, sum_diff))) - write_table(col) - write_fh.write("\n
\n") - - write_pivot_section("MinOpts") - write_pivot_section("FullOpts") + write_pivot_section("Overall", open=True) + write_pivot_section("MinOpts", open=False) + write_pivot_section("FullOpts", open=False) else: write_fh.write("No diffs found.\n") @@ -2128,34 +2123,33 @@ def format_pct(base_instructions, diff_instructions): color = "red" if diff_instructions > base_instructions else "green" return "{}{:.2f}%".format(color, plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) - if any(is_significant(base, diff) for (_, base, diff) in tp_diffs): - def write_table(col): + significant_diffs = {} + for mch_file, base, diff in tp_diffs: + significant_diffs[mch_file] = is_significant(base, diff) + + if any(significant_diffs[mch_file] for (mch_file, _, _) in tp_diffs): + def write_pivot_section(col, open): + write_fh.write("\n\n".format(" open" if open else "")) + sum_base = sum(int(base_metrics[col]["Diff executed instructions"]) for (_, base_metrics, _) in tp_diffs) + sum_diff = sum(int(diff_metrics[col]["Diff executed instructions"]) for (_, _, diff_metrics) in tp_diffs) + + write_fh.write("{} ({})\n\n".format(col, format_pct(sum_base, sum_diff))) write_fh.write("|Collection|PDIFF|\n") write_fh.write("|---|---|\n") for mch_file, base, diff in tp_diffs: base_instructions = int(base[col]["Diff executed instructions"]) diff_instructions = int(diff[col]["Diff executed instructions"]) - if is_significant_pct(base_instructions, diff_instructions): + if significant_diffs[mch_file]: write_fh.write("|{}|{}|\n".format( mch_file, format_pct(base_instructions, diff_instructions))) - write_fh.write("\n") - - write_table("Overall") - - def write_pivot_section(col): - write_fh.write("\n
\n") - sum_base = sum(int(base_metrics[col]["Diff executed instructions"]) for (_, base_metrics, _) in tp_diffs) - sum_diff = sum(int(diff_metrics[col]["Diff executed instructions"]) for (_, _, diff_metrics) in tp_diffs) - - write_fh.write("{} ({})\n\n".format(col, format_pct(sum_base, sum_diff)))) - write_table(col) - write_fh.write("\n
\n") + write_fh.write("\n\n
\n") - write_pivot_section("MinOpts") - write_pivot_section("FullOpts") + write_pivot_section("Overall", open=True) + write_pivot_section("MinOpts", open=False) + write_pivot_section("FullOpts", open=False) else: write_fh.write("No significant throughput differences found\n") From 654a5d97b70c96d63891b9109e2cb3c46beb011a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 5 Sep 2022 17:23:55 +0200 Subject: [PATCH 10/22] Optimized -> FullOpts, switch some wording, switch back to bytes --- src/coreclr/scripts/superpmi.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 1054895d6a1114..bf5b55a4e1af56 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1835,7 +1835,7 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: def format_delta(base, diff): plus_if_positive = "+" if diff >= base else "" color = "red" if diff > base else "green" - return "{}{:,.3f}".format(color, plus_if_positive, (diff - base) / 1000) + return "{}{:,d}".format(color, plus_if_positive, diff - base) diffed_contexts = sum(int(diff_metrics["Overall"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) @@ -1843,7 +1843,7 @@ def format_delta(base, diff): missing_base_contexts = sum(int(base_metrics["Overall"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) missing_diff_contexts = sum(int(diff_metrics["Overall"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - write_fh.write("Diffs are based on {:,d} contexts ({:,d} minopts, {:,d} optimized).\n\n".format( + write_fh.write("Diffs are based on {:,d} contexts ({:,d} MinOpts, {:,d} FullOpts).\n\n".format( diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) color_base = "#d35400" if missing_base_contexts > 0 else "green" @@ -1856,14 +1856,14 @@ def write_pivot_section(col, open): sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) - write_fh.write("{} ({} KB)\n\n".format(col, format_delta(sum_base, sum_diff))) - write_fh.write("|Collection|Base size (KB)|Diff size (KB)|\n") + write_fh.write("{} ({} bytes)\n\n".format(col, format_delta(sum_base, sum_diff))) + write_fh.write("|Collection|Base size (bytes)|Diff size (bytes)|\n") write_fh.write("|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, _) in asm_diffs: if not has_diffs: continue - write_fh.write("|{}|{:,.0f}|{}|\n".format( + write_fh.write("|{}|{:,d}|{}|\n".format( mch_file, int(base_metrics[col]["Diffed code bytes"]), format_delta( @@ -1882,16 +1882,16 @@ def write_pivot_section(col, open): write_fh.write("\n
\n") write_fh.write("Details\n\n") - write_fh.write("|Collection|Diffed contexts|Base missing contexts|Diff missing contexts|Minopts contexts|Optimized contexts|\n") + write_fh.write("|Collection|Diffed contexts|MinOpts contexts|FullOpts contexts|Base missing contexts|Diff missing contexts|\n") write_fh.write("|---|---|---|---|---|---|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: write_fh.write("|{}|{:,d}|{:,d}|{:,d}|{:,d}|{:,d}|\n".format( mch_file, int(diff_metrics["Overall"]["Successful compiles"]), - int(base_metrics["Overall"]["Missing compiles"]), - int(diff_metrics["Overall"]["Missing compiles"]), int(diff_metrics["MinOpts"]["Successful compiles"]), - int(diff_metrics["FullOpts"]["Successful compiles"]))) + int(diff_metrics["FullOpts"]["Successful compiles"]), + int(base_metrics["Overall"]["Missing compiles"]), + int(diff_metrics["Overall"]["Missing compiles"]))) write_fh.write("\n") @@ -2156,7 +2156,7 @@ def write_pivot_section(col, open): write_fh.write("\n
\n") write_fh.write("Details\n\n") - for (disp, col) in [("All", "Overall"), ("MinOpts", "MinOpts"), ("Optimized", "FullOpts")]: + for (disp, col) in [("All", "Overall"), ("MinOpts", "MinOpts"), ("FullOpts", "FullOpts")]: write_fh.write("{} contexts:\n\n".format(disp)) write_fh.write("|Collection|Base # instructions|Diff # instructions|PDIFF|\n") write_fh.write("|---|---|---|---|\n") From 365a0d457f79428dde0e59b5a1c6efe2e2aed9da Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 5 Sep 2022 17:29:58 +0200 Subject: [PATCH 11/22] Right align numbers --- src/coreclr/scripts/superpmi.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index bf5b55a4e1af56..26f1fe83aba279 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1858,7 +1858,7 @@ def write_pivot_section(col, open): write_fh.write("{} ({} bytes)\n\n".format(col, format_delta(sum_base, sum_diff))) write_fh.write("|Collection|Base size (bytes)|Diff size (bytes)|\n") - write_fh.write("|---|---|---|\n") + write_fh.write("|---|--:|--:|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, _) in asm_diffs: if not has_diffs: continue @@ -1883,7 +1883,7 @@ def write_pivot_section(col, open): write_fh.write("Details\n\n") write_fh.write("|Collection|Diffed contexts|MinOpts contexts|FullOpts contexts|Base missing contexts|Diff missing contexts|\n") - write_fh.write("|---|---|---|---|---|---|\n") + write_fh.write("|---|--:|--:|--:|--:|--:|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: write_fh.write("|{}|{:,d}|{:,d}|{:,d}|{:,d}|{:,d}|\n".format( mch_file, @@ -2135,7 +2135,7 @@ def write_pivot_section(col, open): write_fh.write("{} ({})\n\n".format(col, format_pct(sum_base, sum_diff))) write_fh.write("|Collection|PDIFF|\n") - write_fh.write("|---|---|\n") + write_fh.write("|---|--:|\n") for mch_file, base, diff in tp_diffs: base_instructions = int(base[col]["Diff executed instructions"]) diff_instructions = int(diff[col]["Diff executed instructions"]) @@ -2159,7 +2159,7 @@ def write_pivot_section(col, open): for (disp, col) in [("All", "Overall"), ("MinOpts", "MinOpts"), ("FullOpts", "FullOpts")]: write_fh.write("{} contexts:\n\n".format(disp)) write_fh.write("|Collection|Base # instructions|Diff # instructions|PDIFF|\n") - write_fh.write("|---|---|---|---|\n") + write_fh.write("|---|--:|--:|--:|\n") for mch_file, base, diff in tp_diffs: base_instructions = int(base[col]["Diff executed instructions"]) diff_instructions = int(diff[col]["Diff executed instructions"]) From 38a85fb7140d81443365fb1e0a20294b0f1c4545 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 9 Sep 2022 11:19:19 +0200 Subject: [PATCH 12/22] Change some headings --- src/coreclr/scripts/superpmi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 26f1fe83aba279..7929f77e85287b 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1882,7 +1882,7 @@ def write_pivot_section(col, open): write_fh.write("\n
\n") write_fh.write("Details\n\n") - write_fh.write("|Collection|Diffed contexts|MinOpts contexts|FullOpts contexts|Base missing contexts|Diff missing contexts|\n") + write_fh.write("|Collection|Diffed contexts|MinOpts|FullOpts|Missed, base|Missed, diff|\n") write_fh.write("|---|--:|--:|--:|--:|--:|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: write_fh.write("|{}|{:,d}|{:,d}|{:,d}|{:,d}|{:,d}|\n".format( From 366523a4914c854f7025651f14c62220c786c7a7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 13 Sep 2022 16:46:57 +0200 Subject: [PATCH 13/22] Handle JIT switching to minopts/fullopts --- .../tools/superpmi/superpmi/commandline.cpp | 4 --- .../tools/superpmi/superpmi/jitinstance.cpp | 28 ++++++++++++++++--- .../tools/superpmi/superpmi/jitinstance.h | 2 +- .../tools/superpmi/superpmi/superpmi.cpp | 12 +++----- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/coreclr/tools/superpmi/superpmi/commandline.cpp b/src/coreclr/tools/superpmi/superpmi/commandline.cpp index f14df494a3d19c..563d8cb16248c0 100644 --- a/src/coreclr/tools/superpmi/superpmi/commandline.cpp +++ b/src/coreclr/tools/superpmi/superpmi/commandline.cpp @@ -83,10 +83,6 @@ void CommandLine::DumpHelp(const char* program) 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"); diff --git a/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp b/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp index b96fef776fde53..dcf1f8abfe2c69 100644 --- a/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp +++ b/src/coreclr/tools/superpmi/superpmi/jitinstance.cpp @@ -298,7 +298,7 @@ extern "C" DLLEXPORT NOINLINE void Instrumentor_GetInsCount(UINT64* result) } } -JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, MetricsSummary* metrics, CORJIT_FLAGS* jitFlags) +JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, MetricsSummary* metrics, bool* isMinOpts) { struct Param : FilterSuperPMIExceptionsParam_CaptureException { @@ -309,7 +309,7 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i int mcIndex; bool collectThroughput; MetricsSummary* metrics; - CORJIT_FLAGS* jitFlags; + bool* isMinOpts; } param; param.pThis = this; param.result = RESULT_SUCCESS; // assume success @@ -317,7 +317,9 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i param.mcIndex = mcIndex; param.collectThroughput = collectThroughput; param.metrics = metrics; - param.jitFlags = jitFlags; + param.isMinOpts = isMinOpts; + + *isMinOpts = false; // store to instance field our raw values, so we can figure things out a bit later... mc = MethodToCompile; @@ -337,7 +339,14 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i CORINFO_OS os = CORINFO_WINNT; pParam->pThis->mc->repCompileMethod(&pParam->info, &pParam->flags, &os); - pParam->pThis->mc->repGetJitFlags(pParam->jitFlags, sizeof(*pParam->jitFlags)); + CORJIT_FLAGS jitFlags; + pParam->pThis->mc->repGetJitFlags(&jitFlags, sizeof(jitFlags)); + + *pParam->isMinOpts = + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE) || + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT) || + jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); + if (pParam->collectThroughput) { pParam->pThis->lt.Start(); @@ -350,6 +359,17 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i pParam->pThis->lt.Stop(); pParam->pThis->times[0] = pParam->pThis->lt.GetCycles(); } + + CorInfoMethodRuntimeFlags flags = pParam->pThis->mc->cr->repSetMethodAttribs(pParam->info.ftn); + if ((flags & CORINFO_FLG_SWITCHED_TO_MIN_OPT) != 0) + { + *pParam->isMinOpts = true; + } + else if ((flags & CORINFO_FLG_SWITCHED_TO_OPTIMIZED) != 0) + { + *pParam->isMinOpts = false; + } + if (jitResult == CORJIT_SKIPPED) { SPMI_TARGET_ARCHITECTURE targetArch = GetSpmiTargetArchitecture(); diff --git a/src/coreclr/tools/superpmi/superpmi/jitinstance.h b/src/coreclr/tools/superpmi/superpmi/jitinstance.h index 7ff4bd4a7004b0..35b322d172fc58 100644 --- a/src/coreclr/tools/superpmi/superpmi/jitinstance.h +++ b/src/coreclr/tools/superpmi/superpmi/jitinstance.h @@ -60,7 +60,7 @@ class JitInstance bool resetConfig(MethodContext* firstContext); - Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, struct MetricsSummary* metrics, class CORJIT_FLAGS* jitFlags); + Result CompileMethod(MethodContext* MethodToCompile, int mcIndex, bool collectThroughput, struct MetricsSummary* metrics, bool* isMinOpts); const WCHAR* getForceOption(const WCHAR* key); const WCHAR* getOption(const WCHAR* key); diff --git a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp index 1009535c1c7fc3..7c2d44514459a1 100644 --- a/src/coreclr/tools/superpmi/superpmi/superpmi.cpp +++ b/src/coreclr/tools/superpmi/superpmi/superpmi.cpp @@ -367,19 +367,14 @@ int __cdecl main(int argc, char* argv[]) } MetricsSummary baseMetrics; - CORJIT_FLAGS jitFlags; + bool isMinOpts; jittedCount++; st3.Start(); - res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &baseMetrics, &jitFlags); + res = jit->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &baseMetrics, &isMinOpts); st3.Stop(); LogDebug("Method %d compiled%s in %fms, result %d", reader->GetMethodContextIndex(), (o.nameOfJit2 == nullptr) ? "" : " by JIT1", st3.GetMilliseconds(), res); - bool isMinOpts = - jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE) || - jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT) || - jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); - MetricsSummary& totalBaseMetricsOpts = isMinOpts ? totalBaseMetrics.MinOpts : totalBaseMetrics.FullOpts; MetricsSummary& totalDiffMetricsOpts = isMinOpts ? totalDiffMetrics.MinOpts : totalDiffMetrics.FullOpts; @@ -405,8 +400,9 @@ int __cdecl main(int argc, char* argv[]) crl = mc->cr; mc->cr = new CompileResult(); + bool isMinOptsDiff; st4.Start(); - res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &diffMetrics, &jitFlags); + res2 = jit2->CompileMethod(mc, reader->GetMethodContextIndex(), collectThroughput, &diffMetrics, &isMinOptsDiff); st4.Stop(); LogDebug("Method %d compiled by JIT2 in %fms, result %d", reader->GetMethodContextIndex(), st4.GetMilliseconds(), res2); From e58e40421182e9cf13aeb6b68c29fca0ba175bd8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 13 Sep 2022 21:52:37 +0200 Subject: [PATCH 14/22] Revert forward sub change --- src/coreclr/jit/forwardsub.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index eefa48e182705e..f3e4cac3e22716 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -116,13 +116,6 @@ PhaseStatus Compiler::fgForwardSub() } #endif - CLRRandom rng; - rng.Init(info.compILCodeSize ^ info.compILlocalsCount ^ 0x12345678); - if (rng.Next(10) == 0) - { - return PhaseStatus::MODIFIED_NOTHING; - } - bool changed = false; for (BasicBlock* const block : Blocks()) From 17778ad310b22a506aebb0087312b5f842973a04 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 10:54:47 +0200 Subject: [PATCH 15/22] Leave sections unexpanded by default --- src/coreclr/scripts/superpmi.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 7929f77e85287b..ce6d9802525405 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1851,8 +1851,8 @@ def format_delta(base, diff): write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): - def write_pivot_section(col, open): - write_fh.write("\n\n".format(" open" if open else "")) + def write_pivot_section(col): + write_fh.write("\n
\n") sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) @@ -1872,9 +1872,9 @@ def write_pivot_section(col, open): write_fh.write("\n\n
\n") - write_pivot_section("Overall", open=True) - write_pivot_section("MinOpts", open=False) - write_pivot_section("FullOpts", open=False) + write_pivot_section("Overall") + write_pivot_section("MinOpts") + write_pivot_section("FullOpts") else: write_fh.write("No diffs found.\n") @@ -2128,8 +2128,8 @@ def format_pct(base_instructions, diff_instructions): significant_diffs[mch_file] = is_significant(base, diff) if any(significant_diffs[mch_file] for (mch_file, _, _) in tp_diffs): - def write_pivot_section(col, open): - write_fh.write("\n\n".format(" open" if open else "")) + def write_pivot_section(col): + write_fh.write("\n
\n") sum_base = sum(int(base_metrics[col]["Diff executed instructions"]) for (_, base_metrics, _) in tp_diffs) sum_diff = sum(int(diff_metrics[col]["Diff executed instructions"]) for (_, _, diff_metrics) in tp_diffs) @@ -2147,9 +2147,9 @@ def write_pivot_section(col, open): write_fh.write("\n\n
\n") - write_pivot_section("Overall", open=True) - write_pivot_section("MinOpts", open=False) - write_pivot_section("FullOpts", open=False) + write_pivot_section("Overall") + write_pivot_section("MinOpts") + write_pivot_section("FullOpts") else: write_fh.write("No significant throughput differences found\n") From fa45a99e8e770c67b9366624c814bd55c52139f1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 10:57:57 +0200 Subject: [PATCH 16/22] Address feedback --- src/coreclr/scripts/superpmi.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index ce6d9802525405..4c5403a832173b 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1837,11 +1837,17 @@ def format_delta(base, diff): color = "red" if diff > base else "green" return "{}{:,d}".format(color, plus_if_positive, diff - base) - diffed_contexts = sum(int(diff_metrics["Overall"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - diffed_minopts_contexts = sum(int(diff_metrics["MinOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - diffed_opts_contexts = sum(int(diff_metrics["FullOpts"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) - missing_base_contexts = sum(int(base_metrics["Overall"]["Missing compiles"]) for (_, base_metrics, _, _, _) in asm_diffs) - missing_diff_contexts = sum(int(diff_metrics["Overall"]["Missing compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs) + def sum_base(row, col): + return sum(int(base_metrics[row][col]) for (_, base_metrics, _, _, _) in asm_diffs) + + def sum_diff(row, col): + return sum(int(diff_metrics[row][col]) for (_, _, diff_metrics, _, _) in asm_diffs) + + diffed_contexts = sum_diff("Overall", "Successful compiles") + diffed_minopts_contexts = sum_diff("MinOpts", "Successful compiles") + diffed_opts_contexts = sum_diff("FullOpts", "Successful compiles") + missing_base_contexts = sum_base("Overall", "Missing compiles") + missing_diff_contexts = sum_diff("Overall", "Missing compiles") write_fh.write("Diffs are based on {:,d} contexts ({:,d} MinOpts, {:,d} FullOpts).\n\n".format( diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) @@ -1851,12 +1857,12 @@ def format_delta(base, diff): write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): - def write_pivot_section(col): + def write_pivot_section(row): write_fh.write("\n
\n") - sum_base = sum(int(base_metrics[col]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) - sum_diff = sum(int(diff_metrics[col]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) + sum_base = sum(int(base_metrics[row]["Diffed code bytes"]) for (_, base_metrics, _, _, _) in asm_diffs) + sum_diff = sum(int(diff_metrics[row]["Diffed code bytes"]) for (_, _, diff_metrics, _, _) in asm_diffs) - write_fh.write("{} ({} bytes)\n\n".format(col, format_delta(sum_base, sum_diff))) + write_fh.write("{} ({} bytes)\n\n".format(row, format_delta(sum_base, sum_diff))) write_fh.write("|Collection|Base size (bytes)|Diff size (bytes)|\n") write_fh.write("|---|--:|--:|\n") for (mch_file, base_metrics, diff_metrics, has_diffs, _) in asm_diffs: @@ -1865,10 +1871,10 @@ def write_pivot_section(col): write_fh.write("|{}|{:,d}|{}|\n".format( mch_file, - int(base_metrics[col]["Diffed code bytes"]), + int(base_metrics[row]["Diffed code bytes"]), format_delta( - int(base_metrics[col]["Diffed code bytes"]), - int(diff_metrics[col]["Diffed code bytes"])))) + int(base_metrics[row]["Diffed code bytes"]), + int(diff_metrics[row]["Diffed code bytes"])))) write_fh.write("\n\n
\n") From a710c45f8dea7186f68604ba87ad5518b26a7845 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 12:25:14 +0200 Subject: [PATCH 17/22] Revert "Revert forward sub change" This reverts commit e58e40421182e9cf13aeb6b68c29fca0ba175bd8. --- src/coreclr/jit/forwardsub.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index f3e4cac3e22716..eefa48e182705e 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -116,6 +116,13 @@ PhaseStatus Compiler::fgForwardSub() } #endif + CLRRandom rng; + rng.Init(info.compILCodeSize ^ info.compILlocalsCount ^ 0x12345678); + if (rng.Next(10) == 0) + { + return PhaseStatus::MODIFIED_NOTHING; + } + bool changed = false; for (BasicBlock* const block : Blocks()) From e0cc967542dd4424b4ebc444c72103fb93c9f7a9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 12:25:23 +0200 Subject: [PATCH 18/22] Add a horizontal line separator, heading for jit-analyze output --- src/coreclr/scripts/superpmi.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 4c5403a832173b..73d38438cc9fe9 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1902,6 +1902,9 @@ def write_pivot_section(row): write_fh.write("\n") if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): + write_fh.write("---\n\n") + write_fh.write("#### jit-analyze output\n") + for (mch_file, base_metrics, diff_metrics, has_diffs, jit_analyze_summary_file) in asm_diffs: if not has_diffs or jit_analyze_summary_file is None: continue From 3fe712ac1c96d395519f574835f469038f9aa1b4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 15 Sep 2022 15:18:29 +0200 Subject: [PATCH 19/22] Address feedback --- src/coreclr/scripts/superpmi.py | 36 ++++++++++++++----- .../scripts/superpmi_diffs_summarize.py | 4 +-- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 73d38438cc9fe9..157da560093415 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1410,6 +1410,8 @@ def replay(self): # SuperPMI Replay/AsmDiffs ################################################################################ +def html_color(color, text): + return "{}".format(color, text) class SuperPMIReplayAsmDiffs: """ SuperPMI Replay AsmDiffs class @@ -1834,8 +1836,13 @@ async def create_one_artifact(jit_path: str, location: str, flags) -> str: with open(overall_md_summary_file, "w") as write_fh: def format_delta(base, diff): plus_if_positive = "+" if diff >= base else "" - color = "red" if diff > base else "green" - return "{}{:,d}".format(color, plus_if_positive, diff - base) + text = "{}{:,d}".format(plus_if_positive, diff - base) + + if diff != base: + color = "red" if diff > base else "green" + return html_color(color, text) + + return text def sum_base(row, col): return sum(int(base_metrics[row][col]) for (_, base_metrics, _, _, _) in asm_diffs) @@ -1849,12 +1856,19 @@ def sum_diff(row, col): missing_base_contexts = sum_base("Overall", "Missing compiles") missing_diff_contexts = sum_diff("Overall", "Missing compiles") - write_fh.write("Diffs are based on {:,d} contexts ({:,d} MinOpts, {:,d} FullOpts).\n\n".format( - diffed_contexts, diffed_minopts_contexts, diffed_opts_contexts)) + num_contexts_color = "#1460aa" + write_fh.write("Diffs are based on {} contexts ({} MinOpts, {} FullOpts).\n\n".format( + html_color(num_contexts_color, "{:,d}".format(diffed_contexts)), + html_color(num_contexts_color, "{:,d}".format(diffed_minopts_contexts)), + html_color(num_contexts_color, "{:,d}".format(diffed_opts_contexts)))) - color_base = "#d35400" if missing_base_contexts > 0 else "green" - color_diff = "#d35400" if missing_diff_contexts > 0 else "green" - write_fh.write("MISSED contexts: base: {:,d}, diff: {:,d}\n\n".format(color_base, missing_base_contexts, color_diff, missing_diff_contexts)) + missed_color = "#d35400" + base_color = missed_color if missing_base_contexts > 0 else "green" + diff_color = missed_color if missing_diff_contexts > 0 else "green" + write_fh.write("{} contexts: base: {}, diff: {}\n\n".format( + html_color(missed_color, "MISSED"), + html_color(base_color, "{:,d}".format(missing_base_contexts)), + html_color(diff_color, "{:,d}".format(missing_diff_contexts)))) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): def write_pivot_section(row): @@ -2129,8 +2143,12 @@ def check(col): return check("Overall") or check("MinOpts") or check("FullOpts") def format_pct(base_instructions, diff_instructions): plus_if_positive = "+" if diff_instructions > base_instructions else "" - color = "red" if diff_instructions > base_instructions else "green" - return "{}{:.2f}%".format(color, plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) + text = "{}{:.2f}%".format(plus_if_positive, (diff_instructions - base_instructions) / base_instructions * 100) + if diff_instructions != base_instructions: + color = "red" if diff_instructions > base_instructions else "green" + return html_color(color, text) + + return text significant_diffs = {} for mch_file, base, diff in tp_diffs: diff --git a/src/coreclr/scripts/superpmi_diffs_summarize.py b/src/coreclr/scripts/superpmi_diffs_summarize.py index a75d01bd8229e8..4045e2e6bc41e4 100644 --- a/src/coreclr/scripts/superpmi_diffs_summarize.py +++ b/src/coreclr/scripts/superpmi_diffs_summarize.py @@ -90,7 +90,7 @@ def append_diff_file(f, arch, file_name, full_file_path, asmdiffs): diffs_found = True f.write("## {} {}\n".format(diff_os, diff_arch)) f.write(contents) - f.write("\n") + f.write("\n\n---\n\n") return diffs_found @@ -137,7 +137,7 @@ def main(main_args): f.write("No diffs found\n") f.write("\n\n#Throughput impact on Windows {}\n\n".format(arch)) - f.write("The following tables contain the impact on throughput " + + f.write("The following shows the impact on throughput " + "in terms of number of instructions executed inside the JIT. " + "Negative percentages/lower numbers are better.\n\n") From 1a23870c872f75d77370cf15333208d28cfa1018 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 19 Sep 2022 11:13:22 +0200 Subject: [PATCH 20/22] Revert "Revert "Revert forward sub change"" This reverts commit a710c45f8dea7186f68604ba87ad5518b26a7845. --- src/coreclr/jit/forwardsub.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index d20df2eea50b16..7ae30b23826369 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -116,13 +116,6 @@ PhaseStatus Compiler::fgForwardSub() } #endif - CLRRandom rng; - rng.Init(info.compILCodeSize ^ info.compILlocalsCount ^ 0x12345678); - if (rng.Next(10) == 0) - { - return PhaseStatus::MODIFIED_NOTHING; - } - bool changed = false; for (BasicBlock* const block : Blocks()) From 6910d3cda648942f0dfc452adb67e7dd12c0ca4f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 19 Sep 2022 11:17:53 +0200 Subject: [PATCH 21/22] Random comment change to trigger SPMI without diffs --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a40cc67b4519d1..2cf4a417336a77 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -3147,7 +3147,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) { GenTree** parentArgx = &arg.EarlyNodeRef(); - // Morph the arg node, and update the parent and argEntry pointers. + // Morph the arg node and update the node pointer. GenTree* argx = *parentArgx; if (argx == nullptr) { From 25285d5ca1dae5f252a15f91290647eda2ec2e2d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 19 Sep 2022 15:16:12 +0200 Subject: [PATCH 22/22] Hide "missed" contexts line when there are none --- src/coreclr/scripts/superpmi.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 2a7d7e4b7a1994..997451a2aa2c04 100644 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -1884,13 +1884,14 @@ def sum_diff(row, col): html_color(num_contexts_color, "{:,d}".format(diffed_minopts_contexts)), html_color(num_contexts_color, "{:,d}".format(diffed_opts_contexts)))) - missed_color = "#d35400" - base_color = missed_color if missing_base_contexts > 0 else "green" - diff_color = missed_color if missing_diff_contexts > 0 else "green" - write_fh.write("{} contexts: base: {}, diff: {}\n\n".format( - html_color(missed_color, "MISSED"), - html_color(base_color, "{:,d}".format(missing_base_contexts)), - html_color(diff_color, "{:,d}".format(missing_diff_contexts)))) + if missing_base_contexts > 0 or missing_diff_contexts > 0: + missed_color = "#d35400" + base_color = missed_color if missing_base_contexts > 0 else "green" + diff_color = missed_color if missing_diff_contexts > 0 else "green" + write_fh.write("{} contexts: base: {}, diff: {}\n\n".format( + html_color(missed_color, "MISSED"), + html_color(base_color, "{:,d}".format(missing_base_contexts)), + html_color(diff_color, "{:,d}".format(missing_diff_contexts)))) if any(has_diff for (_, _, _, has_diff, _) in asm_diffs): def write_pivot_section(row):