- 
                Notifications
    
You must be signed in to change notification settings  - Fork 5.2k
 
Disable PerfMap generation for Apple mobile platforms #121237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| 
           Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @steveisok, @akoeplinger  | 
    
| 
           /cc: @jkoritzinsky Please let me know if this is good approach  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for Apple platform variants to the PerfMap writer by mapping additional Darwin-based operating systems (MacCatalyst, iOS, iOSSimulator, tvOS, tvOSSimulator) to the existing OSX PerfMapOSToken.
Key Changes
- Maps five additional Apple platform variants to 
PerfMapOSToken.OSXin the switch expression 
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.Diagnostics/PerfMapWriter.cs:126
- The switch expression is now missing a case for 
TargetOS.WebAssemblywhich exists in the TargetOS enum (defined in src/coreclr/tools/Common/TypeSystem/Common/TargetDetails.cs). This will cause a NotImplementedException at runtime when WebAssembly is used as the target OS. Add a case for TargetOS.WebAssembly with an appropriate PerfMapOSToken mapping, or add test coverage to ensure all TargetOS values are handled. 
            PerfMapOSToken osToken = details.OperatingSystem switch
            {
                TargetOS.Unknown => PerfMapOSToken.Unknown,
                TargetOS.Windows => PerfMapOSToken.Windows,
                TargetOS.Linux => PerfMapOSToken.Linux,
                TargetOS.OSX => PerfMapOSToken.OSX,
                TargetOS.MacCatalyst => PerfMapOSToken.OSX,
                TargetOS.iOS => PerfMapOSToken.OSX,
                TargetOS.iOSSimulator => PerfMapOSToken.OSX,
                TargetOS.tvOS => PerfMapOSToken.OSX,
                TargetOS.tvOSSimulator => PerfMapOSToken.OSX,
                TargetOS.FreeBSD => PerfMapOSToken.FreeBSD,
                TargetOS.NetBSD => PerfMapOSToken.NetBSD,
                TargetOS.SunOS => PerfMapOSToken.SunOS,
                _ => throw new NotImplementedException(details.OperatingSystem.ToString())
            };
| 
           Shall we make 
 runtime/src/coreclr/tools/aot/ILCompiler.Diagnostics/ReadyToRunDiagnosticsConstants.cs Line 26 in 98f204b 
  | 
    
| 
           The PerfMap constants are part of the PerfMap format, so any changes would require versioning the format, which isn't worth doing for this case (especially as this format doesn't really make sense in this scenario)  | 
    
          
 Should the right fix be to disable perfmap generation in these scenarios instead then?  | 
    
| 
           The right fix would be to disable generation on the consumer side (ie don't pass the flag to emit PerfMaps). Then we could block passing the flag to emit them for these platforms. The iOS perf tests for CoreCLR aren't well configured at the moment though if they're hitting this. They're requesting ReadyToRun image generation even though it doesn't work as-is.  | 
    
          
 @kotlarmilos Can we do that instead this change?  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR title can be now changed to Disable PerfMap generation for Apple mobile platforms.
| nodeFactoryFlags.PrintReproArgs = Get(_command.PrintReproInstructions); | ||
| nodeFactoryFlags.EnableCachedInterfaceDispatchSupport = Get(_command.EnableCachedInterfaceDispatchSupport); | ||
| 
               | 
          ||
| // Disable PerfMap generation for Apple mobile platforms as they are not supported for ReadyToRun | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be disabled in the script or makefile that calls crossgen instead?
Something is explicitly passing an option to generate a perfmap and we are not able to honor that request. We should print an error that it is not supported here. Silently ignoring command-line options is just causing confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is enabled when PublishReadyToRunEmitSymbols is set:
runtime/src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs
Lines 317 to 335 in 3a8ed8b
| if (!Crossgen2IsVersion5 && _emitSymbols) | |
| { | |
| if (Crossgen2Tool.GetMetadata(MetadataKeys.TargetOS) == "windows") | |
| { | |
| result.AppendLine("--pdb"); | |
| result.AppendLine($"--pdb-path:{Path.GetDirectoryName(_outputPDBImage)}"); | |
| } | |
| else | |
| { | |
| result.AppendLine("--perfmap"); | |
| result.AppendLine($"--perfmap-path:{Path.GetDirectoryName(_outputPDBImage)}"); | |
| string perfmapFormatVersion = Crossgen2Tool.GetMetadata(MetadataKeys.PerfmapFormatVersion); | |
| if (!string.IsNullOrEmpty(perfmapFormatVersion)) | |
| { | |
| result.AppendLine($"--perfmap-format-version:{perfmapFormatVersion}"); | |
| } | |
| } | |
| } | 
PublishReadyToRunEmitSymbols is set unconditionally here:
https://github.com/dotnet/arcade/blob/6247a683b6e52f9c24e9c7b8832bb4cd3537e7f7/src/Microsoft.DotNet.SharedFramework.Sdk/targets/sharedfx.targets#L10
I disabled it for Apple mobile in the targets. For unsupported scenarios, I would prefer to fail compilation rather than handle it gracefully, since it should be fixed rather than worked around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be enough, I think it is set in ReadyToRunCodegenCompilationBuilder as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkoritzinsky,, can we dedup this targets with https://github.com/dotnet/sdk/blob/7538a89c176527c27767a85bdfe0afacef9edeb3/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.CrossGen.targets? Ideally there should be a single source of truth (like @ViktorHofer did for RID graph #92211).
Description
After #121187 Crossgen2 determines cross-compilation targets by reading Crossgen2Tool.GetMetadata(MetadataKeys.TargetOS). This code path triggers PerfMapWriter, which currentl doesn't support iOS targets. This PR disables PerfMap generation for Apple mobile platforms.