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] [iOS] Cumulative Mono iOS HelloWorld size regressions from .NET 9 #105701

Open
matouskozak opened this issue Jul 30, 2024 · 10 comments
Open
Labels
area-VM-meta-mono os-ios Apple iOS size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@matouskozak
Copy link
Member

matouskozak commented Jul 30, 2024

Throughout the .NET 9 development cycle, the iOS HelloWorld Mono size measurements recorded several regressions related to BCL changes:

Regression PR* Size
#93072 #90764 -
#101488 #101258 0.29 MB
#100975 #99982 1.40 MB
#104073 #101196 0.12 MB
#104952 #103837 0.64 MB

The common denominator of the changes was introduction of new generic APIs. I investigated a bit the impact of generic vs specialized API on the iOS app size using an dotnet new ios sample app and simple generic vs specialized interface.

Generic Specialized
interface IFace {
    void foo<T> (T t);
}

class Class1 : IFace {
    public virtual void foo<T> (T t) {
        Console.WriteLine ("Class1.foo<T> called with " + t);
    }
}
interface IFace {
    void foo (int t);
    void foo (string t);
}

class Class1 : IFace {
    public void foo (int t) {
        Console.WriteLine ("Class1.foo<int> called with " + t);
    }

    public void foo (string t) {
        Console.WriteLine ("Class1.foo<string> called with " + t);
    }
}

The preliminary conclusion is that generics API don't bring increase in app size over the specialized API. The results were ~ identical for both apps.

The issue could be to be related to Mono not being able to trim out the newly introduced BCL APIs even though they are not used by the iOS app.


Tested on 9.0.100-preview.5.24307.3

  • The marked PR are selected as the most likely root cause from the regression range. Only in some cases the regression PR was confirmed by manual verification
@matouskozak matouskozak added this to the 9.0.0 milestone Jul 30, 2024
@matouskozak matouskozak changed the title [mono[iOS] Cumulative Mono iOS HelloWorld size regressions from .NET 9 [mono] [iOS] Cumulative Mono iOS HelloWorld size regressions from .NET 9 Jul 30, 2024
@kotlarmilos
Copy link
Member

Thanks for creating this tracking issue. Do we plan to address them in .NET 9?

@matouskozak
Copy link
Member Author

matouskozak commented Jul 30, 2024

Thanks for creating this tracking issue. Do we plan to address them in .NET 9?

Currently unsure, I put .NET 9 milestone to bring attention to this problem. Based on the discussion of possible solutions to this problem we can decide if it's .NET 9 feasible or .NET 10 likely cc: @steveisok @vitek-karas @lambdageek

@vitek-karas
Copy link
Member

My take: We don't have enough time. Any change around this is likely relatively complicated. We're like 1 week away from RC1 so anything complex becomes quite risky. I also think that to do the proper fix we need to spend some time analyzing what it actually means to really fix this. But I don't know the technical details, maybe there's some magical trick we can use :-)

@kotlarmilos
Copy link
Member

re: #79285

This approach seems to involve special handling for Enum, which may not be the best solution overall. Since this doesn't appear to be a reasonable fix / high effort, I will close the issue.

The issue could be to be related to Mono not being able to trim out the newly introduced BCL APIs even though they are not used by the iOS app.

If we don't have clear action points, investigating would probably be high effort and not justifiable impact.

@matouskozak matouskozak modified the milestones: 9.0.0, 10.0.0 Aug 9, 2024
@matouskozak
Copy link
Member Author

If we don't have clear action points, investigating would probably be high effort and not justifiable impact.

I agree it's definitely not .NET 9 target. I would like to see more investigation effort put into this to understand the issue and decide on the action points (if any). Moving to .NET 10 and will re-visit in the next planning phase.

@tannergooding
Copy link
Member

A couple of these might be simple to get fixes for if the cause isn't strictly generics and rather the trimming and such.

For example, #101258 was a correctness fix to ensure the V128 APIs were calling the right throw helper. But because the supported T are consistent across all vector types we could centralize this instead.

If it's 0.29MB per helper here, then consolidating all of Vector<T> and V64/128/256/512<T> to a singular helper could save 1.16MB; maybe more considering how many places use these helpers.

It may similarly be possible to have APIs like Vector64/128/256/512<T>.IsSupported defer to a central IsSupported API, which may allow Mono to deduplicate things. Although I don't know how to validate/check that is the case.


It's notably also a bit unclear why 101258 would cause a regression here, since ThrowForUnsupportedIntrinsicsVector128BaseType should have already been generated from the many other code paths that use it, that presumably would also not be getting trimmed/optimized.

Do we know why this in particular would cause that regression?

@matouskozak
Copy link
Member Author

matouskozak commented Aug 12, 2024

It's notably also a bit unclear why 101258 would cause a regression here, since ThrowForUnsupportedIntrinsicsVector128BaseType should have already been generated from the many other code paths that use it, that presumably would also not be getting trimmed/optimized.

Do we know why this in particular would cause that regression?

As I mentioned in the issue description, the root cause PR is "best guess" so perhaps I was wrong in this case. I'll try to reproduce it locally to confirm.
Edit. Confirmed locally that #101258 is the root cause of #101488 regression.

@kotlarmilos
Copy link
Member

kotlarmilos commented Aug 12, 2024

It may similarly be possible to have APIs like Vector64/128/256/512.IsSupported defer to a central IsSupported API, which may allow Mono to deduplicate things.

Do you have a size estimate for the IsSupported function instance? I assume the size difference would be smaller compared to the throw helper.

@tannergooding
Copy link
Member

Do you have a size estimate for the IsSupported function instance?

I do not, and I'm not familiar with how to query the size regression here. This was more just commenting on a place that could be shared if that sharing might help Mono

Edit. Confirmed locally that #101258 is the root cause of #101488 regression

That's definitely interesting. There's 26 usages of ThrowForUnsupportedIntrinsicsVector128BaseType: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs,1b9e105e20218597,references
and 22 for ThrowForUnsupportedIntrinsicsVector256BaseType: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs,7324b0d17ea7bba5,references

So I wouldn't have expected changing a V128 API from using the latter to using the former instead would be the thing causing the throw helper to be compiled by Mono. Maybe inlining or some other IR transformation is causing the difference here?

@matouskozak matouskozak added the size-reduction Issues impacting final app size primary for size sensitive workloads label Aug 15, 2024
Copy link
Contributor

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-meta-mono os-ios Apple iOS size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

No branches or pull requests

4 participants