[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms - #86971

Merged
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported
Jun 19, 2023
Merged

[mono] Add conditional substitution for IsDynamicCodeSupported when targeting ios-like platforms#86971
ivanpovazan merged 3 commits into
dotnet:mainfrom
ivanpovazan:ios-trim-dyn-code-supported

Conversation

@ivanpovazan

Copy link
Copy Markdown
Member

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

@ivanpovazanivanpovazan self-assigned this May 31, 2023
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ivanpovazanivanpovazan added the os-ios Apple iOS label May 31, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR introduces conditional linker substitution for get_IsDynamicCodeSupported method when targeting ios-like platforms with mono.
The substitution is enabled when feature switch DynamicCodeSupport is set to false, which should only happen in fullAOT mode - with no interpreter fallback.

Contributes to: dotnet/macios#18340

Author:ivanpovazan
Assignees:ivanpovazan
Labels:

os-ios, needs-area-label

Milestone:-

@ivanpovazanivanpovazan added area-Codegen-AOT-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27
not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

@steveisok

steveisok commented May 31, 2023

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

@lewing

Copy link
Copy Markdown
Member

NativeAOT sets the feature switch in the sdk: https://github.com/dotnet/sdk/blob/9ca8336afab6247a22e745c2de4960e9b660d164/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L27 not sure if there is another place (except in Xamarin sdk) to set it for Mono (apart from dotnet/runtime functional testing)

I think once Alex can land a fix for dotnet/sdk#25392, we would have the option of having it set in the SDK. There will likely need to be properties for nativeaot and monoaot.

You can set it in WorkloadManifest.targets now

@lewinglewing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linker substitution doesn't look conditional to me?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

@lewing thanks for your input, I have a couple of questions:

You can set it in WorkloadManifest.targets now

The linker substitution doesn't look conditional to me?

  • feature="System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" featurevalue="false" makes it conditional in the sense that the method get_IsDynamicCodeSupported() will be replaced with false by the trimmer if the DynamicCodeSupport feature switch is set to false. Otherwise, there will be no substitution.

@marek-safar

Copy link
Copy Markdown
Contributor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

@ivanpovazan

ivanpovazan commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

This needs to be wired up into some internal msbuild property otherwise it has no effect because we don't want anyone to change this manually.

Agreed.
I propose as a mid-term solution to set DynamicCodeSupport around here: https://github.com/xamarin/xamarin-macios/blob/cc35efe4bc46f2aa32a0190356cedaad2723e266/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L122 once this PR lands.
As a long-term solution, it will be set in the SDK once the work dotnet/sdk#25392 is completed.

Do you agree @marek-safar ?

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/cc: @lambdageek

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan

Copy link
Copy Markdown
MemberAuthor

Failures seem unrelated

Comment threadsrc/tests/FunctionalTests/iOS/Device/IsDynamicCodeSupported/Program.cs Outdated
@ivanpovazan

ivanpovazan commented Jun 16, 2023

Copy link
Copy Markdown
MemberAuthor

I have removed the previously added test as this set up is not unit testable for which I have considered:

  1. Using reflection to test whether a code path guarded by RuntimeFeature.IsDynamicCodeSupported is preserved; but in that case ILLink will preserve dependencies that are referenced dynamically (through reflection) defeating the purpose of the test
  2. Manually setting AppContext.SetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", true); and making sure it does not have any effect. However, in that case MonoAOT actually does not look into managed side (and the AppContext) when determining the value for RuntimeFeature.IsDynamicCodeSupported, instead it performs an instrinsic optimisation:
    // On FullAOT, return false for RuntimeFeature:
    // - IsDynamicCodeCompiled
    // - IsDynamicCodeSupported and no interpreter
    // otherwise use the C# code in System.Private.CoreLib
    if (in_corlib&&
    cfg->full_aot&&
    !strcmp ("System.Runtime.CompilerServices", cmethod_klass_name_space) &&
    !strcmp ("RuntimeFeature", cmethod_klass_name)) {
    if (!strcmp (cmethod->name, "get_IsDynamicCodeCompiled")) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    } elseif (!strcmp (cmethod->name, "get_IsDynamicCodeSupported") && !cfg->interp) {
    EMIT_NEW_ICONST (cfg, ins, 0);
    ins->type=STACK_I4;
    returnins;
    }
    }

None of the approaches seems feasible.


Finally, as this change only affects app size, the verification and impact should be observable in size/perf measurements.
This will also open opportunities for further size optimisations like:

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I agree testing this change here is not really feasible at this time. Maybe if we had "dotnet-linker-tests" for iOS working, it could be possible.

@ivanpovazan
ivanpovazan merged commit 2975717 into dotnet:mainJun 19, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jul 19, 2023
@ivanpovazan
ivanpovazan deleted the ios-trim-dyn-code-supported branch August 15, 2023 09:44
Sign up for freeto 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.

8 participants

@ivanpovazan@steveisok@lewing@marek-safar@vargaz@filipnavara@eerhardt@MichalStrehovsky