Skip to content

Conversation

@am11
Copy link
Member

@am11 am11 commented Nov 9, 2022

Bring them to the same plan as dotnet/runtime#72082 and dotnet/runtime#76467.

Remaining two are jit-diff and jit-format. I have left them (for now ™️),
as it turned out to be quite a feat to migrate jit-diff to this plan. 😁

@jakobbotsch
Copy link
Member

Thank you! We will probably need to do a little testing with this before we can merge it.

Comment on lines 25 to 26
public Option<List<string>> Metrics { get; } =
new(new[] { "--metrics", "-m" }, _ => new List<string> { "CodeSize" }, true, $"Comma-separated metric to use for diff computations. Available metrics: {MetricCollection.ListMetrics()}");
Copy link
Member

Choose a reason for hiding this comment

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

Is it still true that this can be specified with comma-separated syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. :)
I have reworded it to reflect unlimited arity of argument.

Also, _ => <const>, true/false was incorrect for frozen default values, replaced with () => <const>.

Copy link
Member

Choose a reason for hiding this comment

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

Note that superpmi.py in dotnet/runtime is passing this arg so it will need changes too.

It does introduce some weirdness if you try to run superpmi on the servicing branches, but I'm not sure if we really need to worry about it (running superpmi.py on servicing branches is rare, and so is specifying custom metrics to superpmi.py). I would be fine with changing the syntax here, but also cc @dotnet/jit-contrib for a chance to express their concerns.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to submit the fix for this too? Specifically around here: https://github.com/dotnet/runtime/blob/db717e30839c532cad5b269ac11c5d2c91dad639/src/coreclr/scripts/superpmi.py#L1790-L1791
Looks like the help message for the -metrics arg in superpmi.py needs an update too.

@jakobbotsch jakobbotsch self-requested a review November 14, 2022 18:32
@am11
Copy link
Member Author

am11 commented Nov 21, 2022

Bumped version (based on dotnet/runtime#78577).

@jakobbotsch
Copy link
Member

Bumped version (based on dotnet/runtime#78577).

Thanks! I hope to take a look at and do some testing of this PR this week.

@jakobbotsch
Copy link
Member

jakobbotsch commented Nov 24, 2022

The first problem I hit is that System.CommandLine.dll published into the bin dir gets overridden by the System.CommandLine.dll, so after ./bootstrap.cmd the executables output to bin that use the old System.CommandLine.dll do not work.

I'll see if there's an easy way to rename one of the DLLs

@am11
Copy link
Member Author

am11 commented Nov 24, 2022

Publishing as single-file resolved the conflicting dependencies issue when publishing to the same directory (which is generally not recommended): 95056b2.

@jakobbotsch
Copy link
Member

Publishing as single-file resolved the conflicting dependencies issue when publishing to the same directory (which is generally not recommended): 95056b2.

That should be fine, but I think we need to skip it for PMI that is meant to be loaded into a runtime with a checked JIT.

@am11
Copy link
Member Author

am11 commented Nov 25, 2022

I've skipped PMI from PublishSingleFile.

but I think we need to skip it for PMI that is meant to be loaded into a runtime with a checked JIT.

Just curious, what prevents it from working with singlefilehost? Do we unload/reload the corelib which is not possible with single-file mode or something else?

@jakobbotsch
Copy link
Member

jakobbotsch commented Dec 11, 2022

Just curious, what prevents it from working with singlefilehost? Do we unload/reload the corelib which is not possible with single-file mode or something else?

The idea of PMI is to execute it with a custom build of the runtime/JIT. It then loads a bunch of assemblies and forcefully JITs lots of functions, which allows producing the disassembly for these functions so that it can be diffed and compared. So we generally don't want to use it with the public runtime that single file would entail.

BTW, sorry for the delay here -- I hope to test this change more asap.

(EDIT: Actually I just realized I've been using this PR for my personal jitutils install for the past 2 weeks without issues, so that's a good sign 😄)

@jakobbotsch
Copy link
Member

FWIW, jit-dasm-pmi can be singlefile. Only pmi needs to be framework dependent.

@am11
Copy link
Member Author

am11 commented Dec 11, 2022

Thanks for testing it @jakobbotsch. :)

I have updated the condition to only exclude src/pmi from PublishSingleFile.

@am11
Copy link
Member Author

am11 commented Jan 11, 2023

Thanks for the fixes, @jakobbotsch!

If possible, I think it would be nice to move jitutils under https://github.com/dotnet/runtime/tree/main/src/tools and use live build in the CI; diffs legs, for example, can build these tools spending two (or less) additional minutes per run. That will save round-trips and keep things up-to-date with convenience.

Edit: it currently clones this repo and use the live build, so moving code to runtime repo will actually save a few seconds. 😅

@jakobbotsch
Copy link
Member

Thanks for the fixes, @jakobbotsch!

No problem, thanks for the help! Sorry for the slowness. I think this is good to go now, I did some last tests and things look ok for me locally now.

If possible, I think it would be nice to move jitutils under https://github.com/dotnet/runtime/tree/main/src/tools and use live build in the CI; diffs legs, for example, can build these tools spending two (or less) additional minutes per run. That will save round-trips and keep things up-to-date with convenience.

Edit: it currently clones this repo and use the live build, so moving code to runtime repo will actually save a few seconds. 😅

It has been discussed quite a few times, but so far it has not amounted to something we want to spend time on. There are both good arguments for and against, and if we did it we would probably only move some of the tools since this repo serves as kind of a kitchen sink for a lot of very unpolished JIT tools that we don't want to bloat dotnet/runtime with.

@jakobbotsch jakobbotsch merged commit 58a02f1 into dotnet:main Jan 11, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this pull request Jan 18, 2023
jit-analyze no longer accepts comma-separated metrics after
dotnet/jitutils#362, instead requiring each
metric to be specified separately on the command line.
jakobbotsch added a commit to dotnet/runtime that referenced this pull request Jan 18, 2023
jit-analyze no longer accepts comma-separated metrics after
dotnet/jitutils#362, instead requiring each
metric to be specified separately on the command line.
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
jit-analyze no longer accepts comma-separated metrics after
dotnet/jitutils#362, instead requiring each
metric to be specified separately on the command line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants