Skip to content
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

[Apple mobile] Enable trimming on build machines to match ILLink features #110966

Merged
merged 15 commits into from
Jan 13, 2025

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Dec 27, 2024

Description

This PR moves trimming from Helix to AzDo (build) machines to simplify ILLink feature alignment between AzDo and Helix. Previously, when an ILLink feature was added to the SDK, we needed to manually maintain an internal setup. This led to regressions, and time spend on investigation to identify the root cause, such as in:
#103594
#110771

Changes

Trimming is now moved trimming from Helix to AzDo (build) machines. Redundant code for maintaining ILLink features is removed.

This change introduces a 20min regression in build time for the complete set of library tests on runtime-extra-platforms pipeline, as they are now trimmed sequentially on build machines. However, it is expected to save time when investigating regressions caused by SDK changes.

Verification

The changes have been verified on the runtime and runtime-extra-platforms pipelines.

Fixes #110771 #104431 #110266

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 27, 2024
@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos kotlarmilos added os-ios Apple iOS and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 27, 2024
@kotlarmilos kotlarmilos added this to the 10.0.0 milestone Dec 27, 2024
@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

Does this mean that each job that is building full library tests has 20 min slower build time or overall runtime-extra-platforms pipeline build got 20 min slower (combined across all library test jobs)?

Each job that is building full library tests on Apple mobile. It impacts the total build time of runtime-extra-platforms but reduces maintenance costs.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

I left couple of comments, but all in all neat work!

@@ -16,7 +16,7 @@
<!-- running aot-helix tests locally, so we can test with the same project file as CI -->
<_AOTBuildCommand Condition="'$(ContinuousIntegrationBuild)' != 'true'">$(_AOTBuildCommand) /p:RuntimeSrcDir=$(RepoRoot) /p:RuntimeConfig=$(Configuration)</_AOTBuildCommand>
<!-- The command below sets default properties for runtime and library tests -->
<_AOTBuildCommand>$(_AOTBuildCommand) /p:XHARNESS_EXECUTION_DIR=&quot;$XHARNESS_EXECUTION_DIR&quot; /p:RunAOTCompilation=$(RunAOTCompilation) /p:UseNativeAOTRuntime=$(UseNativeAOTRuntime) /p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:MonoForceInterpreter=$(MonoForceInterpreter) /p:MonoEnableLLVM=true /p:DevTeamProvisioning=$(DevTeamProvisioning) /p:UsePortableRuntimePack=true /p:Configuration=$(Configuration) /p:EnableAggressiveTrimming=$(EnableAggressiveTrimming)</_AOTBuildCommand>
<_AOTBuildCommand>$(_AOTBuildCommand) /p:XHARNESS_EXECUTION_DIR=&quot;$XHARNESS_EXECUTION_DIR&quot; /p:RunAOTCompilation=$(RunAOTCompilation) /p:UseNativeAOTRuntime=$(UseNativeAOTRuntime) /p:TargetOS=$(TargetOS) /p:TargetArchitecture=$(TargetArchitecture) /p:MonoForceInterpreter=$(MonoForceInterpreter) /p:MonoEnableLLVM=true /p:DevTeamProvisioning=$(DevTeamProvisioning) /p:UsePortableRuntimePack=false /p:Configuration=$(Configuration) /p:EnableAggressiveTrimming=false</_AOTBuildCommand>
Copy link
Member

Choose a reason for hiding this comment

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

Since we are not trimming on Helix anymore, do we need to set EnableAggressiveTrimming for AOTing at all?

@@ -11,6 +11,7 @@
<IncludesTestRunner>false</IncludesTestRunner>
<ExpectedExitCode>42</ExpectedExitCode>
<EnableAggressiveTrimming>true</EnableAggressiveTrimming>
<DynamicCodeSupport>false</DynamicCodeSupport>
Copy link
Member

Choose a reason for hiding this comment

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

I assume this setting needs to be there because we are testing the full AOT mode?
It would be good to move it to some common .props/targets file instead as adding new tests for testing full AOT could miss setting this feature switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Updated condition in tests.mobile.targets to override default trimming features.

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

Once CI is green I think this is good to go, thanks!

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/ba-g timeouts

@kotlarmilos kotlarmilos merged commit ff5726f into dotnet:main Jan 13, 2025
183 of 194 checks passed
@matouskozak
Copy link
Member

Is this change low risk to backport it to net 9 branch to improve the CI there?

@kotlarmilos
Copy link
Member Author

This is a low risk change.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 15, 2025
@kotlarmilos
Copy link
Member Author

/backport to release/9.0-staging

@github-actions github-actions bot unlocked this conversation Mar 13, 2025
Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13832612248

Copy link
Contributor

@kotlarmilos backporting to "release/9.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Trim on build machine
Applying: Remove unknown types from a descriptor file
Applying: Don't trim on Helix
Using index info to reconstruct a base tree...
M	eng/pipelines/extra-platforms/runtime-extra-platforms-ioslike.yml
Falling back to patching base and 3-way merge...
Auto-merging eng/pipelines/extra-platforms/runtime-extra-platforms-ioslike.yml
CONFLICT (content): Merge conflict in eng/pipelines/extra-platforms/runtime-extra-platforms-ioslike.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0003 Don't trim on Helix
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono][tvos] DataSet Xml serialization tests failing on tvos lanes
3 participants