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][perf] iOS and WASM disk size regressions on 06 Dec 2022 #79285

Closed
kotlarmilos opened this issue Dec 6, 2022 · 15 comments
Closed

[mono][perf] iOS and WASM disk size regressions on 06 Dec 2022 #79285

kotlarmilos opened this issue Dec 6, 2022 · 15 comments
Labels
area-VM-meta-mono size-reduction Issues impacting final app size primary for size sensitive workloads tenet-performance Performance related issue
Milestone

Comments

@kotlarmilos
Copy link
Member

kotlarmilos commented Dec 6, 2022

Description

Change in Enum values #78580 produced disk size regression in Mono iOS sample app and in Mono Blazorwasm.

Binary disk size (SOD) for Mono iOS sample has increased by 9.86%.
Framework disk size (SOD) for Mono AOT Blazorwasm sample has increased by 9.48%.

Details - Mono iOS sample app

Filename Parent commit SOD in bytes (6e0c046) Change commit SOD in bytes (62f3eb2)
HelloiOS 18998464 21078752
Info.plist 980 980
PkgInfo 8 8
Program.aotdata 4792 4792
Program.deps.json 11929 11929
Program.dll 6656 6656
Program.runtimeconfig.json 1064 1064
System.Console.aotdata 63344 138632
System.Console.dll 17920 17920
System.Private.CoreLib.aotdata 930368 1118880
System.Private.CoreLib.dll 1015808 1047552
icudt.dat 1859040 1859040

Screenshot 2022-12-13 at 11 39 04


Details - Mono WASM

Date Diff Change Size increase (bytes) Size increase (%)
06 Dec 2022 b399eed...fcf0a09 62f3eb2 11042 0.51

Screenshot 2022-12-13 at 10 14 54


Details - Mono AOT WASM

Date Diff Change Size increase (bytes) Size increase (%)
06 Dec 2022 b399eed...fcf0a09 62f3eb2 414367 9.48

Screenshot 2022-12-13 at 11 42 43

cc @SamMonoRT @lewing @radekdoulik

Configuration

iOS

OS: iOS/OSX
Architecture: arm64
.NET version: 7.0.100-rc.1.22431.12

WASM

OS: Ubuntu 18.04
Architecture: x64
.NET version: 7.0.100-rc.1.22431.12

@kotlarmilos kotlarmilos added the tenet-performance Performance related issue label Dec 6, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 6, 2022
@stephentoub
Copy link
Member

Do we know where exactly the increase comes from?

@lewing
Copy link
Member

lewing commented Dec 6, 2022

wasm report is here dotnet/perf-autofiling-issues#10461 at first glance it looks like the growth is primarily in System.Private.CoreLib and retained native code.

@stephentoub
Copy link
Member

How does one determine what native code is being retained that wasn't before, and why it's being retained?

@SamMonoRT SamMonoRT added the size-reduction Issues impacting final app size primary for size sensitive workloads label Dec 8, 2022
@ghost
Copy link

ghost commented Dec 8, 2022

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

Issue Details

Description

Change in Enum values #78580 produced disk size regression in Mono iOS and WASM.

Binary disk size (SOD) for Mono iOS sample has increased by 9.86%.
Binary disk size (SOD) for Mono Blazorwasm sample has increased by 11.58%.

Mono iOS details

Filename SOD in bytes (6e0c046) SOD in bytes (62f3eb2)
HelloiOS 18998464 21078752
Info.plist 980 980
PkgInfo 8 8
Program.aotdata 4792 4792
Program.deps.json 11929 11929
Program.dll 6656 6656
Program.runtimeconfig.json 1064 1064
System.Console.aotdata 63344 138632
System.Console.dll 17920 17920
System.Private.CoreLib.aotdata 930368 1118880
System.Private.CoreLib.dll 1015808 1047552
icudt.dat 1859040 1859040

Mono Blazorwasm details

Filename SOD in bytes (6e0c046) SOD in bytes (62f3eb2)
dotnet.wasm.br 3091674 3496616

cc @SamMonoRT @lewing @radekdoulik

Configuration

OS: iOS/OSX
Architecture: arm64
.NET version: 7.0.100-rc.1.22431.12

Author: kotlarmilos
Assignees: -
Labels:

tenet-performance, untriaged, area-VM-meta-mono, size-reduction

Milestone: -

@SamMonoRT SamMonoRT removed the untriaged New issue has not been triaged by the area owner label Dec 8, 2022
@SamMonoRT SamMonoRT added this to the 8.0.0 milestone Dec 8, 2022
@kotlarmilos
Copy link
Member Author

I don't think that we can analyze a dependency graph. I suggest to compare native symbols before and after the change. Moreover, we can check ILLinker logs for any trimming/linking actions made before and after the change.

Generics TUnderlyingValue could have impacted the size, especially with change mentioned in #78851.

@stephentoub
Copy link
Member

@jkotas, FYI

@ivanpovazan
Copy link
Member

How does one determine what native code is being retained that wasn't before, and why it's being retained?

@stephentoub, afaik currently there is no easy way to do this with Mono. We usually go through the changes and compare outputs as @kotlarmilos suggested. Regarding this, I have opened #73159 as a .NET 8 idea which would help us answer such questions and improve the overall experience for detecting these kind of issues.

This being said, maybe we could utilize NativeAOT compiler's ILScanner mode, which we could run before and after a change in libraries' code, to dump the dependency graphs and track down what are all the new dependencies introduced by the change.

@eerhardt
Copy link
Member

eerhardt commented Dec 8, 2022

Maybe https://github.com/google/bloaty might help here? Not sure if it can be used on iOS, but I believe I've heard of people using it for WASM before.

@ivanpovazan
Copy link
Member

Maybe https://github.com/google/bloaty might help here? Not sure if it can be used on iOS, but I believe I've heard of people using it for WASM before.

I agree @eerhardt, I have used it along with a custom script to try to compare two separate binaries explained here:
#56385 (comment)
If further explanation is needed on how to collect the measurements, I would gladly provide some more info.

@jkotas
Copy link
Member

jkotas commented Dec 8, 2022

System.Private.CoreLib.dll IL size grew 30kB (1015808 to 1047552). System.Private.CoreLib.aotdata grew 184kB (930368 to 1118880). It means that there is disproportionally large growth in AOT size.

System.Console.dll IL size have not changed and there are no changes in System.Console in this PR. System.Console.aotdata grew 75kB, that is more than doubled in size (63344 to 138632).

This suggests that that the bulk of the regression is in generic expansion or inlining of something that is used by System.Console. My guess is that the bulk of the regression was triggered by https://github.com/dotnet/runtime/pull/78580/files#diff-b731ce4281dee295e9c8ad712cc211b7c8a0f9afee1a79e2a6bf248192fcd657R309-R320 . The AOT compiler may be expanding the enum path for every T and not just for enums.

@marek-safar
Copy link
Contributor

I agree with @jkotas this looks like limitation/bug in Mono AOT engine.

/cc @vargaz to confirm

@MichalStrehovsky
Copy link
Member

Seeing an 8% regression (for Hello World) in NativeAOT as well: #79437.

@vargaz
Copy link
Contributor

vargaz commented Dec 9, 2022

Part of the increase seems to be caused by code like this:

            Type underlyingType = typeof(TEnum).GetEnumUnderlyingType();

                if (underlyingType == typeof(int)) return TryFormatPrimitiveDefault(rt, *(int*)&value, destination, out charsWritten);
                if (underlyingType == typeof(uint)) return TryFormatPrimitiveDefault(rt, *(uint*)&value, destination, out charsWritten);

Mono currently doesn't treat GetEnumUnderlyingType as an intrinsics. Even if that is implemented, it's not going to be smart enough to realize that 'underlyingType' points to typeof(int) at the if () location, so it can't optimize out the if () statements, forcing it to generate all the TryFormatPrimitiveDefault instances.

@kotlarmilos kotlarmilos changed the title Mono iOS and WASM disk size regressions [mono][perf] iOS and WASM disk size regressions on 06 Dec 2022 Dec 13, 2022
@kotlarmilos
Copy link
Member Author

Follow-up research on size improvements in Mono AOT compiler -- it seems that inflated instances in addition to gsharedvt created the regression with changes in Enum values.

Bloaty symbols diff before and after the change - https://gist.github.com/kotlarmilos/a5c7c0b3372f0e019a9e2016ce7805e5

It seems that single TryParseByValueOrName instance allocates about 14kb. If inflated instances are disabled for TryParseByValueOrName, size improvement is about 300kb. If inflated instances are disabled for System.Enum, size improvement is about 1.6mb.

@vargaz Is it something that we might consider as an option if a speed isn't affected much, or is it not an option?

Bundles are attached for further comparison.

before-enum-bundle.zip
after-enum-bundle.zip

@kotlarmilos
Copy link
Member Author

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.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono size-reduction Issues impacting final app size primary for size sensitive workloads tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

10 participants