[mono][ios] Drop marshal-ilgen from iOS builds if it is not needed #88903

jandupej · 2023-07-14T10:54:19Z

Addresses #88820 and contributes to #61685. RuntimeMarshalingScanner is used to analyze the assemblies and determine if marhsal-lightweight is sufficient for them. If so, the marshal-ilgen component is not included in the package. This should save approx. 44 KB in package size.

Additionally, the iOS HelloWorld sample is fixed so that it does not require marshal-ilgen. This was the only assembly that needed it, from the entire application.

Note: there are still some debug outputs to be removed before merging

…orrect order relative to building the native runtime.

ivanpovazan · 2023-07-14T12:37:29Z

src/mono/sample/iOS/Program.cs

-    private static extern void ios_set_text(string value);
+    unsafe private static extern void ios_set_text(byte* value);
+    //private static extern void ios_set_text(string value);


Could we have these under an #if LIGHTWEIGHT_MARSHALLER preprocessor directive, which can be controlled from the sample project file via:

<DefineConstants Condition="'$(UseLightweightMarshallingFriendlyCode)' == 'true'">LIGHTWEIGHT_MARSHALLER;$(DefineConstants)</DefineConstants>

This way the sample can still be testable for both implementations (full and lightweight) by specifying -p:UseLightweightMarshallingFriendlyCode=true during build.

NOTE: Maybe the naming I used is not the best

This is certainly doable, however I think the point is that the inclusion (or not) of marhsal-ilgen should be automatic. So whenever someone tests code in HelloWorld which requires it, it should kick in by by itself. However it could be a good idea to have a test which marshals something nonblittable to check whether the marshaller is correctly included.

Maybe we can have such test(s) in the: https://github.com/dotnet/runtime/tree/main/src/tests/FunctionalTests

The scenario appears to be covered already, this test https://github.com/dotnet/runtime/blob/main/src/tests/FunctionalTests/iOS/Simulator/AOT-LLVM/Program.cs should fail if the marshaller is not properly included.

ivanpovazan · 2023-07-14T12:43:40Z

src/mono/msbuild/apple/build/AppleBuild.targets

+    <ItemGroup Condition="'@(MonoLightweightMarshallerIncompatibleAssemblies->Count())' > 0">
+      <RuntimeComponents Include="marshal-ilgen" />
+    </ItemGroup>
+    <ItemGroup Condition="'@(MonoLightweightMarshallerIncompatibleAssemblies->Count())' == 0">
+      <RuntimeComponents Remove="marshal-ilgen" />
+    </ItemGroup>


Is there an assumption here that RuntimeComponents is non-empty and can already include marshal-ilgen component before reaching this target?
If yes, wouldn't we then encounter a duplication in case <RuntimeComponents Include="marshal-ilgen" /> on line 101?

Oh, Include does not check for duplicates?

Unfortunately... no :)

Apparently one can use KeepDuplicates on ItemGroups. Who knew

ivanpovazan · 2023-07-14T12:55:53Z

src/mono/msbuild/apple/build/AppleBuild.props

@@ -26,6 +26,8 @@
        _InitializeCommonProperties;
        _BeforeAppleBuild;
        _AppleResolveReferences;
+        _ScanAssembliesDecideLightweightMarshaler;
+        _ProcessRuntimeComponents;


NIT: I think _ProcessRuntimeComponents target could be refactored a bit, especially as it has a single condition _IsLibraryMode on an item group at https://github.com/dotnet/runtime/pull/88903/files#diff-e351a598f31c0a001f31622d9babc297dbcbe5827eabf55ab92700e1839e70f1R109

Proposed changes:

Add a property depending on library mode builds

<PropertyGroup Condition="'$(_IsLibraryMode)' == 'true'"> <_ProcessRuntimeComponentsForLibraryMode>_ProcessRuntimeComponentsForLibraryMode</_ProcessRuntimeComponentsForLibraryMode> </PropertyGroup>

Adjust the AppleBuildDependsOn targets

<AppleBuildDependsOn> _InitializeCommonProperties; _BeforeAppleBuild; _AppleResolveReferences; _ScanAssembliesDecideLightweightMarshaler; $(_ProcessRuntimeComponentsForLibraryMode); _AppleAotCompile; ...

Rename the _ProcessRuntimeComponents target into _ProcessRuntimeComponentsForLibraryMode

Remove the condition on the item group on line: https://github.com/dotnet/runtime/pull/88903/files#diff-e351a598f31c0a001f31622d9babc297dbcbe5827eabf55ab92700e1839e70f1R109

kotlarmilos · 2023-07-14T13:35:45Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-07-14T13:35:51Z

No commit pushedDate could be found for PR 88903 in repo dotnet/runtime

kotlarmilos · 2023-07-14T13:36:07Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-07-14T13:36:13Z

No commit pushedDate could be found for PR 88903 in repo dotnet/runtime

kotlarmilos

The maccatalyst failure seems related. It would be good to check the maccatalyst in sandbox environment as it uses dynamic linking.

Otherwise apart from Ivan's comments, it looks good!

jandupej · 2023-07-14T13:43:18Z

Yes, both the iOS and catalyst failures are related. It seems that the analyzer correctly detects the need for marshal-ilgen, but RuntimeComponents is maybe modified too late into the build process. Need to investigate.

kotlarmilos · 2023-07-14T14:16:17Z

iOS/tvOS failures could be #88909.

jandupej · 2023-07-14T15:04:01Z

/azp list

azure-pipelines · 2023-07-14T15:04:07Z

CI/CD Pipelines for this repository: runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo Antigen runtime-community Fuzzlyn runtime-coreclr superpmi-replay runtime-wasm runtime-coreclr superpmi-diffs runtime-coreclr superpmi-asmdiffs-checked-release runtime-extra-platforms jit-cfg runtime-wasm-perf runtime-llvm runtime-coreclr jitstress-random runtime-coreclr libraries-jitstress-random runtime-android-grpc-client-tests runtime-wasm-libtests runtime-wasm-non-libtests runtime-android runtime-androidemulator runtime-ioslike runtime-ioslikesimulator runtime-linuxbionic runtime-maccatalyst runtime-coreclr pgostress runtime-coreclr jitstress-isas-avx512 runtime-tools-tests runtime-libraries-mono outerloop runtime-sanitized runtime-wasm-dbgtests runtime-wasm-optional

jandupej · 2023-07-14T15:04:43Z

/azp run runtime-ioslike

azure-pipelines · 2023-07-14T15:04:55Z

Azure Pipelines successfully started running 1 pipeline(s).

jandupej · 2023-07-17T09:19:33Z

All CI failures now seem to be known issues.

src/mono/msbuild/apple/build/AppleBuild.targets

ivanpovazan

LGTM! Thank you!

lambdageek

LGTM

jandupej added 5 commits June 28, 2023 11:12

MarshallingPInvokeScanner is applied to Apple toolchains.

89b89d4

HelloiOS example uses simpler string marshaling.

01ff43d

Merge branch 'main' into marshal-ilgen-ios

8302d60

Removing marshal-ilgen from HelloWorld.

c37ef23

Runtime components and runtime library paths are now decided in the c…

696a656

…orrect order relative to building the native runtime.

jandupej added the area-Build-mono label Jul 14, 2023

jandupej added this to the 8.0.0 milestone Jul 14, 2023

jandupej requested review from steveisok, lambdageek, kotlarmilos and ivanpovazan July 14, 2023 10:54

jandupej requested a review from marek-safar as a code owner July 14, 2023 10:54

jandupej self-assigned this Jul 14, 2023

jandupej linked an issue Jul 14, 2023 that may be closed by this pull request

[mono][aot] Disable marshal-ilgen on iOS when not needed #88820

Closed

ivanpovazan reviewed Jul 14, 2023

View reviewed changes

kotlarmilos reviewed Jul 14, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jul 14, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

RuntimeComponents is manipulated through an ItemGroup.

88df373

This was referenced Jul 14, 2023

Tracking issue for CI build timeouts #76454

Closed

[mono][ios] EventPipe processinfo3 test is failing on iOS-like platforms #88186

Closed

Cleaned up commented old code.

8fb47b8

lambdageek reviewed Jul 17, 2023

View reviewed changes

src/mono/msbuild/apple/build/AppleBuild.targets Outdated Show resolved Hide resolved

jandupej added 2 commits July 24, 2023 12:30

Reordered runtime components section.

6d8f939

Addressed more feedback.

2dd6ac0

ivanpovazan approved these changes Jul 24, 2023

View reviewed changes

lambdageek approved these changes Jul 24, 2023

View reviewed changes

jandupej merged commit c8ad025 into dotnet:main Jul 24, 2023

ghost locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono][ios] Drop marshal-ilgen from iOS builds if it is not needed #88903

[mono][ios] Drop marshal-ilgen from iOS builds if it is not needed #88903

jandupej commented Jul 14, 2023 •

edited

Loading

ivanpovazan Jul 14, 2023 •

edited

Loading

jandupej Jul 14, 2023

ivanpovazan Jul 14, 2023

jandupej Jul 17, 2023 •

edited

Loading

ivanpovazan Jul 14, 2023

jandupej Jul 14, 2023

ivanpovazan Jul 14, 2023

jandupej Jul 14, 2023

ivanpovazan Jul 14, 2023

jandupej Jul 24, 2023

kotlarmilos commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

kotlarmilos commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

kotlarmilos left a comment

jandupej commented Jul 14, 2023

kotlarmilos commented Jul 14, 2023

jandupej commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

jandupej commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

jandupej commented Jul 17, 2023

ivanpovazan left a comment

lambdageek left a comment

[mono][ios] Drop marshal-ilgen from iOS builds if it is not needed #88903

[mono][ios] Drop marshal-ilgen from iOS builds if it is not needed #88903

Conversation

jandupej commented Jul 14, 2023 • edited Loading

ivanpovazan Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandupej Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

kotlarmilos commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

kotlarmilos left a comment

Choose a reason for hiding this comment

jandupej commented Jul 14, 2023

kotlarmilos commented Jul 14, 2023

jandupej commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

jandupej commented Jul 14, 2023

azure-pipelines bot commented Jul 14, 2023

jandupej commented Jul 17, 2023

ivanpovazan left a comment

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

jandupej commented Jul 14, 2023 •

edited

Loading

ivanpovazan Jul 14, 2023 •

edited

Loading

jandupej Jul 17, 2023 •

edited

Loading