SPMI: Improve speed significantly for large diffs #76238
Conversation
This starts communicating more information about diffs back from superpmi and starts using it in the driver. The current information is the base size, diff size, base instructions, diff instructions and context size. The driver uses the base size/diff size to pick a small number of interesting contexts and only creates disassembly for these contexts. jit-analyze is then also invoked on only these contexts, but the contexts are picked so that they overlap with what jit-analyze would display. Additionally, we also pick the top 20 smallest contexts (in terms of context size) -- I frequently use the smallest contexts with diffs to investigate my change. The new behavior is only enabled when no custom metrics are specified. If custom metrics are specified we fall back to producing all disassembly files and leave it up to jit-analyze to extract and analyze the metrics specified. The net result is that we can now get SPMI diffs for changes with even hundreds of thousands of diffs in the same time as it takes to get diffs for a small change. Fix dotnet#76178
This is no longer necessary since we avoid creating these disassembly files in the first place. Another benefit is that all contexts mentioned by jit-analyze output will now be part of the artifacts.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis starts communicating more information about diffs back from The driver uses the base size/diff size to pick a small number of The new behavior is only enabled when no custom metrics are specified. The net result is that we can now get SPMI diffs for changes with even Fix #76178
|
| @@ -171,7 +181,8 @@ int __cdecl main(int argc, char* argv[]) | |||
| #endif | |||
|
|
|||
| bool collectThroughput = false; | |||
| MCList failingToReplayMCL, diffMCL; | |||
| MCList failingToReplayMCL; | |||
There was a problem hiding this comment.
It will likely be useful to give the failing list the same treatment so that we can have superpmi.py replay print the smallest contexts with failures, instead of printing everything. I will leave this for a future PR however.
Coercing integer to pointer in constexpr context is invalid. Clang happily miscompiles the 'invalid handle check' to a null pointer check.
|
Very nice to see it. |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Hopefully I can get to see diffs for #76017 and #76185 with this change. |
|
The linux-arm64 job seems to have hung during the .mch download. The superpmi replay is failing on arm32 due to the morph change, seems like a bug in either lowering or LSRA: [22:19:58] ISSUE: <ASSERT> #271154 D:\a\_work\1\s\src\coreclr\jit\lsra.cpp (11934) - Assertion failed 'refPosition->RegOptional()' in 'System.Numerics.Tests.ComplexTests:Abs(double,double)' during 'LSRA build intervals' (IL size 22; hash 0xbf3bc3d1; FullOpts)I'll try a different change with large diffs. |
|
Clean run for a JIT change that has diffs in around 100k contexts: https://dev.azure.com/dnceng-public/public/_build/results?buildId=33036&view=ms.vss-build-web.run-extensions-tab |
Can we have an issue for this? |
Opened #76382. |
|
Ping @AndyAyersMS (or maybe @kunalspathak / @BruceForstall want to take a look?) |
|
|
||
| // This is a difference in ASM outputs from Jit1 & Jit2 and not a playback failure | ||
| // We will add this MC to the diffs info if there is one. | ||
| // Otherwise this will end up in failingMCList |
There was a problem hiding this comment.
I find this a bit puzzling. Can you say more about why we would add this to the failing MCL?
There was a problem hiding this comment.
This is just preserving the existing behavior (it was inside InvokeNearDiffer before).
I'm not sure why we are regarding diffs as failures when -diffMCList (-diffsInfo after this change) is unspecified.
|
This is great work; thanks for doing this. |
| subproc_helper.run_to_completion(create_replay_artifacts, self, mch_file, asm_complus_vars_full_env, text_differences, base_asm_location, diff_asm_location, ".dasm") | ||
|
|
||
| if self.coreclr_args.diff_jit_dump: | ||
| logging.info("Creating JitDump files: %s %s", base_dump_location, diff_dump_location) | ||
| subproc_helper.run_to_completion(create_replay_artifacts, self, mch_file, jit_dump_complus_vars_full_env, jit_dump_differences, base_dump_location, diff_dump_location, ".txt") | ||
|
|
||
| logging.info("Differences found. To replay SuperPMI use:") | ||
| logging.info("Differences found. To replay SuperPMI use:".format(len(diffs))) | ||
|
|
There was a problem hiding this comment.
What is this ".format..."? Shouldn't it use the logging '%s' format? And I don't see any replacement token.
There was a problem hiding this comment.
logging uses the same conventions as the old % formatting operator. This is the newer str.format, which I think would normally be preferred... except that logging probably isn't going to format the string unless the particular logging level has been specified.
(and yes, it is missing {} or {0})
There was a problem hiding this comment.
Whoops yes, at one point I was printing the number of diffs as part of this line, but I decided to print it on a new line at the end of each collection diff so that it gets printed last instead, and you can see it while waiting for the next diffs.
I'll keep in mind to remove this with my next change.
There was a problem hiding this comment.
It looks like logging can be switched to the new formatter: https://docs.python.org/3/howto/logging-cookbook.html#using-particular-formatting-styles-throughout-your-application
That would maybe be a nice cleanup at some point.
There was a problem hiding this comment.
Ah, looks like 'style' came with 3.2. Not sure what version of Python is on the CI systems, but presumably better than that.
There was a problem hiding this comment.
Yes, I (and some others) have been using new style formatting in various scripts for a while now without issues.
ghost
locked as resolved and limited conversation to collaborators
This starts communicating more information about diffs back from
superpmi and starts using it in the driver. The current information is
the base size, diff size, base instructions, diff instructions and
context size.
The driver uses the base size/diff size to pick a small number of
interesting contexts and only creates disassembly for these contexts.
jit-analyze is then also invoked on only these contexts, but the
contexts are picked so that they overlap with what jit-analyze would
display. Additionally, we also pick the 20 smallest contexts (in
terms of context size) -- I frequently use the smallest contexts with
diffs to investigate my changes.
The new behavior is only enabled when no custom metrics are specified.
If custom metrics are specified we fall back to producing all
disassembly files and leave it up to jit-analyze to extract and analyze
the metrics specified.
Also, the retainTopFilesOnly option is no longer necessary since our CI
pipeline will at most produce 100 .dasm files now. Another benefit is that
this means that all contexts mentioned in the jit-analyze output will now be
part of the artifacts.
The net result is that we can now get SPMI diffs for changes with even
hundreds of thousands of diffs in the same time as it takes to get diffs
for a small change.
Fix #76178