Allow jit to disregard PGO; PGO changes for SPMI and MCS #49551

AndyAyersMS · 2021-03-12T19:59:23Z

COMPlus_JitDisablePGO will now block the jit from using profile data. Useful
for investigations and (someday) bug triage.
-jitoption now works for superpmi.py asmdiffs. It only applies to
the base jit.
Limit SPMI file size path component to 50 chars, so that SPMI can handle
being run in directories with long paths we can't control (as happens with
tools like crank).
Enhance mcs -jitflags output to describe how many method contexts have
PGO data, and which kinds of data they hold. Useful for validating that an
SPMI collection done via dynamic PGO has actually collected an interesting
set of jit requests.

* COMPlus_JitDisablePGO will now block the jit from using profile data. Useful for investigations and (someday) bug triage. * `-jitoption` now works for `superpmi.py asmdiffs`. It only applies to the base jit. * Limit SPMI file size path component to 50 chars, so that SPMI can handle being run in directories with long paths we can't control (as happens with tools like `crank`). * Enhance `mcs -jitflags` output to describe how many method contexts have PGO data, and which kinds of data they hold. Useful for validating that an SPMI collection done via dynamic PGO has actually collected an interesting set of jit requests.

AndyAyersMS · 2021-03-12T20:01:01Z

cc @dotnet/jit-contrib

On the newly-uploaded aspnet SPMI collection:

Grouped Flag Appearances (35487 contexts)

bits                count  percent  parsed
0000000048820000      250    0.70%  FEATURE_SIMD SKIP_VERIFICATION IL_STUB BBOPT
0000000248820000      134    0.38%  FEATURE_SIMD SKIP_VERIFICATION IL_STUB BBOPT PUBLISH_SECRET_PARAM
0000006248820000        1    0.00%  FEATURE_SIMD SKIP_VERIFICATION IL_STUB BBOPT PUBLISH_SECRET_PARAM REVERSE_PINVOKE TRACK_TRANSITIONS
0000008000820010     3742   10.54%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION TIER0
0000008020820010    17085   48.14%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBINSTR TIER0
0000000040820010     2950    8.31%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT
0000010040820010     5221   14.71%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT TIER1
c000010040820010     4419   12.45%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT TIER1 HAS_PGO HAS_EDGE_PROFILE
e000010040820010     1582    4.46%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT TIER1 HAS_PGO HAS_EDGE_PROFILE HAS_CLASS_PROFILE
0000008080820010        5    0.01%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION FRAMED TIER0
00000080a0820010       54    0.15%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBINSTR FRAMED TIER0
00000000c0820010        5    0.01%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT FRAMED
00000100c0820010       26    0.07%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT FRAMED TIER1
c0000100c0820010       12    0.03%  DEBUG_INFO FEATURE_SIMD SKIP_VERIFICATION BBOPT FRAMED TIER1 HAS_PGO HAS_EDGE_PROFILE
0000000000820030        1    0.00%  DEBUG_INFO MIN_OPT FEATURE_SIMD SKIP_VERIFICATION

Individual Flag Appearances

   35102   98.92%  DEBUG_INFO
       1    0.00%  MIN_OPT
   35487  100.00%  FEATURE_SIMD
   35487  100.00%  SKIP_VERIFICATION
     385    1.08%  IL_STUB
   17139   48.30%  BBINSTR
   14600   41.14%  BBOPT
     102    0.29%  FRAMED
     135    0.38%  PUBLISH_SECRET_PARAM
       1    0.00%  REVERSE_PINVOKE
       1    0.00%  TRACK_TRANSITIONS
   20886   58.86%  TIER0
   11260   31.73%  TIER1
    1582    4.46%  HAS_CLASS_PROFILE
    6013   16.94%  HAS_EDGE_PROFILE
    6013   16.94%  HAS_PGO

BruceForstall · 2021-03-13T01:18:56Z

src/coreclr/scripts/superpmi.py

@@ -1722,6 +1722,11 @@ def replay_with_asm_diffs(self):
        altjit_asm_diffs_flags = target_flags
        altjit_replay_flags = target_flags

+        option_flags = []
+        if self.coreclr_args.jitoption:
+            for o in self.coreclr_args.jitoption:


I'm not fond of the user model here, and the asymmetry that you can specify options for the base but not diff.

I would suggest having options -jitoption_base and -jitoption_diff, only for the asmdiffs command, so you can support passing these to either base or jit or both. SuperPMI itself only has -jitoption and -jitoption2 because it doesn't associate any semantics between the JITs, but superpmi.py does have a semantic associated with the order we pass JITs to superpmi.exe.

Comments?

Good idea; done. Though for some reason I used -base_jit_option and -diff_jit_option.

BruceForstall · 2021-03-13T01:29:38Z

src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h

+
+enum EXTRA_JIT_FLAGS
+{
+    HAS_PGO = 63,


I wonder if you should put a note in corjitflags.h and jitee.h that you're doing this. Or, put a reference here (like a static compile-time assert) using JIT_FLAG_UNUSED34, JIT_FLAG_UNUSED35, JIT_FLAG_UNUSED36, so if anyone changes those, we'll get a build error here.

BruceForstall · 2021-03-13T22:23:59Z

src/coreclr/scripts/superpmi.py

@@ -294,6 +294,8 @@
 asm_diff_parser.add_argument("--diff_jit_dump", action="store_true", help="Generate JitDump output for diffs. Default: only generate asm, not JitDump.")
 asm_diff_parser.add_argument("-temp_dir", help="Specify a temporary directory used for a previous ASM diffs run (for which --skip_cleanup was used) to view the results. The replay command is skipped.")
 asm_diff_parser.add_argument("--gcinfo", action="store_true", help="Include GC info in disassembly (sets COMPlus_JitGCDump/COMPlus_NgenGCDump; requires instructions to be prefixed by offsets).")
+asm_diff_parser.add_argument("-base_jit_option", action="append", help="Option to pass to the baselne JIT. Format is key=value, where key is the option name without leading COMPlus_...")


I like these names better than my original suggestion

BruceForstall · 2021-03-13T22:24:26Z

src/coreclr/scripts/superpmi.py

+        diff_option_flags = []
+        if self.coreclr_args.diff_jit_option:
+            for o in self.coreclr_args.diff_jit_option:
+                diff_option_flags += "-jitoption2", o


The superpmi.exe flag is actually -jit2option

BruceForstall · 2021-03-13T22:27:12Z

src/coreclr/scripts/superpmi.py

-                            base_txt = await create_one_artifact(self.base_jit_path, base_location)
-                            diff_txt = await create_one_artifact(self.diff_jit_path, diff_location)
+                            base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + base_option_flags)
+                            diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags + diff_option_flags)


This isn't going to work for creating the asm files for diffs, because it will pass -jit2option (when that is fixed), however, we're only passing a single JIT in the "create dasm file artifacts" case (as opposed to the "do asm diffs by passing two JITs and -a). So you need (with better names) diff_option_flags_for_asmdiffs using -jit2option and diff_option_flags_for_generating_dasm using -jitoption.

Ah, ok. I only tried it for the base jit... should have tried it with the diff jit too.

BruceForstall

LGTM

AndyAyersMS · 2021-03-16T01:14:48Z

OSX not happy with my file name hackery.

ERROR: GetResultFileName - path to the output file is too long '/var/folders/wq/wf7g6cm104n7dtsnpz6mmwvm0000gy/T/ahnuom05.whfSPMI\..mc(77)'
{0x264c5c-0x10ee63880} ASSERT [FILE   ] at 
/Users/runner/work/1/s/src/coreclr/pal/src/file/file.cpp.930: WideCharToMultiByte failure! error is 87
/private/tmp/helix/working/AD88094A/w/AFC80A05/e/JIT/superpmi/superpmicollect/Bytemark/Bytemark.sh: line 388: 19599 Trace/BPT trap: 5

AndyAyersMS · 2021-03-16T02:57:28Z

Seems like the logic here is wonky, if fileNameLength is zero... (this is the original code, not the PR)

runtime/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp

Lines 156 to 198 in ec5d2ee

    
           // All lengths in this function exclude the terminal NULL. 
        
           WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const WCHAR* extension) 
        
           { 
        
               const size_t folderPathLength   = wcslen(folderPath); 
        
               const size_t fileNameLength     = wcslen(fileName); 
        
               const size_t extensionLength    = wcslen(extension); 
        
               const size_t maxPathLength      = MAX_PATH - 50; // subtract 50 because excel doesn't like paths longer then 230. 
        
               const size_t randomStringLength = 8; 
        
               size_t fullPathLength   = folderPathLength + 1 + extensionLength; 
        
               bool appendRandomString = false; 
        
               if (fileNameLength > 0) 
        
               { 
        
                   fullPathLength += fileNameLength; 
        
               } 
        
               else 
        
               { 
        
                   fullPathLength += randomStringLength; 
        
                   appendRandomString = true; 
        
               } 
        
               size_t charsToDelete = 0; 
        
               if (fullPathLength > maxPathLength) 
        
               { 
        
                   // The path name is too long; creating the file will fail. This can happen because we use the command line, 
        
                   // which for ngen includes lots of environment variables, for example. 
        
                   // Shorten the file name and add a random string to the end to avoid collisions. 
        
                   charsToDelete = fullPathLength - maxPathLength + randomStringLength; 
        
                   if (fileNameLength >= charsToDelete) 
        
                   { 
        
                       appendRandomString = true; 
        
                       fullPathLength = maxPathLength; 
        
                   } 
        
                   else 
        
                   { 
        
                       LogError("GetResultFileName - path to the output file is too long '%ws\\%ws.%ws(%d)'", folderPath, fileName, extension, fullPathLength); 
        
                       return nullptr; 
        
                   } 
        
               }

AndyAyersMS · 2021-03-16T03:20:28Z

Probably simplest to resolve relative paths so we don't need to guess how long things are.

AndyAyersMS · 2021-03-17T15:19:03Z

@BruceForstall I redid the file name generation logic, feel free to take another look.

sandreenko · 2021-03-24T01:14:36Z

src/coreclr/scripts/superpmi.py

@@ -1859,7 +1879,7 @@ async def create_replay_artifacts(print_prefix, item, self, mch_file, env_vars,
                        # as the LoadLibrary path will be relative to the current directory.
                        with ChangeDir(self.coreclr_args.core_root):

-                            async def create_one_artifact(jit_path: str, location: str) -> str:
+                            async def create_one_artifact(jit_path: str, location: str, flags: list[str]) -> str:


Note that flags: list[str] won't work with Python3.8 or lower, list was changed to be generic in 3.9.
It is probably not a big deal that some narrow parts need a higher python version than the minimal supported for the repo.

We do run collections using this script on Helix machines which have 3.5 (I think), and merge/upload on the "build" machines which have something different (3.6?), so it could be a problem. But this is replay, which is only done on local user machines.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 12, 2021

BruceForstall suggested changes Mar 13, 2021

View reviewed changes

AndyAyersMS added 2 commits March 13, 2021 08:35

review feedback; also disable GDV if we're disabling PGO

46f4e36

fix build issue with clang

e3b2c06

BruceForstall suggested changes Mar 13, 2021

View reviewed changes

rework options handling for asmdiffs

05501b6

BruceForstall approved these changes Mar 15, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 16, 2021

AzDO provisioning issues with hosted machines #44220

Closed

revise file name creation

2c6a24b

BruceForstall approved these changes Mar 17, 2021

View reviewed changes

AndyAyersMS merged commit 1ba22eb into dotnet:main Mar 17, 2021

AndyAyersMS deleted the JitDisablePgo branch March 17, 2021 17:58

sandreenko reviewed Mar 24, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Apr 23, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Allow jit to disregard PGO; PGO changes for SPMI and MCS #49551

Allow jit to disregard PGO; PGO changes for SPMI and MCS #49551

Uh oh!

Conversation

AndyAyersMS commented Mar 12, 2021

Uh oh!

AndyAyersMS commented Mar 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Mar 16, 2021

Uh oh!

AndyAyersMS commented Mar 16, 2021

Uh oh!

AndyAyersMS commented Mar 16, 2021

Uh oh!

AndyAyersMS commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!