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.

Sign up for GitHub

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

Jump to bottom

[mono][tests] Enable ILStrip after AOT compilation for library tests #88167

Merged
merged 17 commits into from
Jan 30, 2024
Merged

[mono][tests] Enable ILStrip after AOT compilation for library tests #88167

merged 17 commits into from
Jan 30, 2024

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Jun 28, 2023 •
edited
Loading

Description

This PR enables IL stripping after AOT compilation for library tests on iOS-like platforms.

Contributes to #87740

@kotlarmilos kotlarmilos added this to the 8.0.0 milestone Jun 28, 2023
@kotlarmilos kotlarmilos requested a review from akoeplinger June 28, 2023 21:57
@kotlarmilos kotlarmilos requested a review from marek-safar as a code owner June 28, 2023 21:57
@kotlarmilos kotlarmilos self-assigned this Jun 28, 2023
@ghost
Copy link

ghost commented Jun 28, 2023

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR aims to enable IL stripping during AOT compilation of library tests on iOS-like platforms.

Fixes #87740

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-Infrastructure-mono, os-ios
Milestone: 8.0.0

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Jun 29, 2023
Open
Closed
Closed
Closed
@kotlarmilos kotlarmilos mentioned this pull request Jun 29, 2023
Closed
@kotlarmilos
Copy link
Member Author

The failures in the library tests appear to be unrelated (TCP connection failure), but since all tests fail during the startup, I would like to check it locally. @akoeplinger Please take a look when you get a chance.

@akoeplinger akoeplinger mentioned this pull request Jul 5, 2023
Merged
@akoeplinger
Copy link
Member

With the fix from https://github.com/dotnet/runtime/pull/87923/files#r1253418906 this passes System.Runtime.Tests on my iOS device, except for three tests which try to assert that a method body contains a specific IL instruction, or inlining attribute etc. I'll fix those and push to this PR.

@lambdageek
Copy link
Member

@akoeplinger We'll need to backport the ILStrip fix to net7.0, too

@akoeplinger
Copy link
Member

Yep, the 7.0 backport is in #88437

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kotlarmilos

This comment was marked as outdated.

Sign in to view
This was referenced Jul 11, 2023
Closed
Closed
@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jul 19, 2023
Closed
@kotlarmilos kotlarmilos changed the title [mono][tests] Enable ILStrip during AOT compilation of library tests [WIP][mono][tests] Enable ILStrip during AOT compilation of library tests Dec 19, 2023
@kotlarmilos kotlarmilos added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 19, 2023
@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos kotlarmilos changed the title [WIP][mono][tests] Enable ILStrip during AOT compilation of library tests [mono][tests] Enable ILStrip during AOT compilation of library tests Dec 21, 2023
@kotlarmilos kotlarmilos changed the title [mono][tests] Enable ILStrip during AOT compilation of library tests [mono][tests] Enable ILStrip after AOT compilation for library tests Dec 21, 2023
@kotlarmilos kotlarmilos removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 21, 2023
@kotlarmilos kotlarmilos marked this pull request as ready for review December 21, 2023 14:21
akoeplinger
akoeplinger reviewed Jan 16, 2024
View reviewed changes
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Did you run runtime-extra-platforms or runtime-ioslike pipelines on this to make sure we don't have additional tests failing?

I wonder a bit if we should only turn this on for a subset of assemblies. Or maybe only for the System.* assemblies.

...s/System.Runtime/tests/System.Diagnostics.Debug.Tests/DebugTestsNoListeners.Interpolation.cs Outdated Show resolved Hide resolved
@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

I wonder a bit if we should only turn this on for a subset of assemblies. Or maybe only for the System.* assemblies.

Let's check if there are other failures. Are there any performance-related implications we need to consider?

@akoeplinger
Copy link
Member

Are there any performance-related implications we need to consider?

Not that I know of. One thing to keep in mind is that we're now also turning this on for library mode i.e. external customers.

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

1 similar comment
@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike

@kotlarmilos
Copy link
Member Author

Failures shouldn't be related. Additionally, I've disabled it for library mode.

@kotlarmilos kotlarmilos merged commit 10ea547 into dotnet:main Jan 30, 2024
181 of 190 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees

@kotlarmilos kotlarmilos

Labels
area-Infrastructure-mono os-ios Apple iOS
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kotlarmilos @akoeplinger @lambdageek