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

[mono][aot] Deduplicate runtime invoke wrappers on iOS #85908

Conversation

kotlarmilos
Copy link
Member

This PR deduplicates runtime invoke wrappers on iOS, which aligns WASM and iOS implementations. Approximately 30 runtime invoke wrappers have been deduplicated, resulting in minimal size savings. According to the HelloiOS measurements, the reduction amounts to about 2kb or 0.01%, which could be within the margin of statistical error.

Such change had an issue with the CI in #84304, even though the tests passed locally.

Fixes #83973

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-extra-platforms

@kotlarmilos kotlarmilos added this to the 8.0.0 milestone May 8, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kotlarmilos
Copy link
Member Author

The System.Runtime tests are failing randomly at different points on the ios-arm64 CI, but they are passing when run locally on a device and on the tvos-arm64 CI. Considering that we are still in the early preview stage, I suggest two possible options:

  • Close the PR and don't consider deduplication of the runtime invoke wrappers. This means that the iOS implementation will remain inconsistent with the WASM implementation.
  • Merge the PR and if the issue continues to occur on the CI, revert the PR.

/cc: @steveisok @SamMonoRT @vargaz

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@SamMonoRT
Copy link
Member

The System.Runtime tests are failing randomly at different points on the ios-arm64 CI, but they are passing when run locally on a device and on the tvos-arm64 CI. Considering that we are still in the early preview stage, I suggest two possible options:

  • Close the PR and don't consider deduplication of the runtime invoke wrappers. This means that the iOS implementation will remain inconsistent with the WASM implementation.
  • Merge the PR and if the issue continues to occur on the CI, revert the PR.

/cc: @steveisok @SamMonoRT @vargaz

Are the random failures happening only as part of your PR, or have we seen those random failures outside your PR? If only in PR, I say try to add more logging and see if we can pinpoint any relevant fixes.

@kotlarmilos
Copy link
Member Author

Are the random failures happening only as part of your PR, or have we seen those random failures outside your PR? If only in PR, I say try to add more logging and see if we can pinpoint any relevant fixes.

These failures are also present in other pipeline runs, always reporting the TCP failure. In this PR, the following pipelines fail, which could indicate that the issue could be with the tests scheduling or networking:

  • runtime - ios-arm64
  • ioslike - tvos-arm64
  • extra-platforms - tvos-arm64 and ios-arm64

However, upon retrying the runs, the distribution of failures remains unchanged, which leads to a suspicion that the issue might be related. There are no other error logs indicating that the runtime can't find the runtime invoke wrappers.

@kotlarmilos
Copy link
Member Author

Test retries indicate that the failures occur randomly. Based on the test logs, it is observed that the failures do not follow any pattern and can happen randomly during the test suite execution, without being tied to any particular test.

@kotlarmilos
Copy link
Member Author

@akoeplinger @steveisok I think you could provide valuable feedback as you have encountered the TCP failure in the past.

@kotlarmilos kotlarmilos merged commit 4258272 into dotnet:main May 24, 2023
@kotlarmilos
Copy link
Member Author

I've merged this PR. If we encounter more frequent failures following the merge, I will revert the change.

/cc: @akoeplinger @steveisok

@kotlarmilos
Copy link
Member Author

We have encountered more frequent failures following the merge, so I suggest we revert the change and try to reproduce the issue locally using the same version of macOS and iPhone as on the CI environment.

Screenshot 2023-06-01 at 22 39 18

@steveisok
Copy link
Member

Agreed, I think this change does have an impact on CI and needs reverted.

kotlarmilos added a commit that referenced this pull request Jun 2, 2023
steveisok pushed a commit that referenced this pull request Jun 2, 2023
…" (#87066)

We have encountered more frequent failures following the merge as outlined in #85908 (comment), so we will revert the change and try to reproduce the issue locally using the same version of macOS and iPhone as on the CI environment.

Reverts #85908
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
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][aot] Further optimizations for dedup improvement
4 participants