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

[Perf] NAOT iOS HelloWorld size: 4 Regressions on 5/15/2024 8:03:38 AM #104074

Closed
performanceautofiler bot opened this issue May 15, 2024 · 13 comments
Closed

Comments

@performanceautofiler
Copy link

performanceautofiler bot commented May 15, 2024

Run Information

Name Value
Architecture x64
OS Mac OS X 10.18
Queue IPhone
Baseline 3de068c2be232960f4cb3b233dd2b4014e2ce7c3
Compare 714a4420805ed53c311b05381c83c88894100fa9
Diff Diff
Configs CompilationMode:tiered, HybridGlobalization:true, iOSStripSymbols:true, RunKind:ios_scenarios, RuntimeType:nativeaot

Regressions in SOD - iOS HelloWorld NativeAOT .app Size nollvm nosymbols

Benchmark Baseline Test Test/Base Test Quality Edge Detector Baseline IR Compare IR IR Ratio
2.76 MB 2.89 MB 1.05 0.00 True
2.76 MB 2.89 MB 1.05 0.00 True
2.76 MB 2.89 MB 1.05 0.00 True
2.76 MB 2.89 MB 1.05 0.00 True

graph
graph
graph
graph
graph
graph
graph
graph
Test Report

Repro

General Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md

git clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f net8.0 --filter 'SOD - iOS HelloWorld NativeAOT .app Size nollvm nosymbols*'

SOD - iOS HelloWorld NativeAOT .app Size nollvm nosymbols

ETL Files

Histogram

JIT Disasms

Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

@matouskozak
Copy link
Member

matouskozak commented May 15, 2024

The range for regression is 778cc84...a71a85a. Could it be #102183 @MichalStrehovsky? ... but seems unlikely that adding assembly name would cause 5% regression in size...

@MichalStrehovsky
Copy link
Member

The range for regression is dotnet/runtime@778cc84...a71a85a. Could it be dotnet/runtime#102183 @MichalStrehovsky? ... but seems unlikely that adding assembly name would cause 5% regression in size...

If this is measuring size with symbols, it would be from that. Symbols should be stripped before measuring size though.

@matouskozak
Copy link
Member

The range for regression is dotnet/runtime@778cc84...a71a85a. Could it be dotnet/runtime#102183 @MichalStrehovsky? ... but seems unlikely that adding assembly name would cause 5% regression in size...

If this is measuring size with symbols, it would be from that. Symbols should be stripped before measuring size though.

This one should be without symbols (nosymbols in the issue header). With symbols we also saw regression of 6% (slightly bigger than this one) dotnet/perf-autofiling-issues#34369.

@MichalStrehovsky
Copy link
Member

I've kicked off a size validation job in MichalStrehovsky/rt-sz#22 (results will be there in ~20 minutes) just to be sure, but that change should only affect symbols (and if it's from that, we should have seen an improvement in January when the regression my PR is fixing was introduced).

@matouskozak
Copy link
Member

matouskozak commented May 16, 2024

I've kicked off a size validation job in MichalStrehovsky/rt-sz#22 (results will be there in ~20 minutes) just to be sure, but that change should only affect symbols (and if it's from that, we should have seen an improvement in January when the regression my PR is fixing was introduced).

Looks like the size validation didn't show any regression. I wonder if it could be something mobile + NAOT specific as it doesn't show in you validation job yet the range doesn't contain anything else related to NAOT.

@MichalStrehovsky
Copy link
Member

I've kicked off a size validation job in MichalStrehovsky/rt-sz#22 (results will be there in ~20 minutes) just to be sure, but that change should only affect symbols (and if it's from that, we should have seen an improvement in January when the regression my PR is fixing was introduced).

Looks like the size validation didn't show any regression. I wonder if it could be something mobile + NAOT specific as it doesn't show in you validation job yet the range doesn't contain anything else related to NAOT.

This should really only affect mangled names and mangled names are only used for symbols. Is it possible to get the binary that this is measuring? Just the after binary should be enough to rule out bad symbol stripping configuration.

@matouskozak
Copy link
Member

@LoopedBard3 do you think it is possible to get the binaries? I found the Perf build https://dev.azure.com/dnceng/internal/_build/results?buildId=2451389&view=logs&j=4f320d25-c1ea-5d1a-2cd7-ab8121a51286 but the Artifacts only contains binlogs.

@matouskozak matouskozak transferred this issue from dotnet/perf-autofiling-issues Jun 27, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 27, 2024
@matouskozak matouskozak changed the title [Perf] Windows/x64: 4 Regressions on 5/15/2024 8:03:38 AM [Perf] NAOT iOS HelloWorld size: 4 Regressions on 5/15/2024 8:03:38 AM Jun 27, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@matouskozak
Copy link
Member

fyi: @ivanpovazan

@MichalStrehovsky
Copy link
Member

@LoopedBard3 do you think it is possible to get the binaries? I found the Perf build https://dev.azure.com/dnceng/internal/_build/results?buildId=2451389&view=logs&j=4f320d25-c1ea-5d1a-2cd7-ab8121a51286 but the Artifacts only contains binlogs.

I think the binaries are right there:

Click the "Download iOS Sample App" step.

Downloading artifact iOSSampleAppNoSymbolsHybridGlobalizationTrue from: https://dev.azure.com/dnceng//_apis/resources/Containers/17496355?itemPath=iOSSampleAppNoSymbolsHybridGlobalizationTrue&isShallow=true&api-version=4.1-preview.4

Click that, follow the JSON to https://dev.azure.com/dnceng/_apis/resources/Containers/17496355?itemPath=iOSSampleAppNoSymbolsHybridGlobalizationTrue%2FiOSSampleAppNoSymbolsHybridGlobalizationTrue.zip.

And sure enough, the binary is not stripped properly. There should not be any S_P_CoreLib strings in a stripped binary, but there's lots of them. This is from #102183, but it's a benchmark setup issue; stripping doesn't do the right thing.

@ivanpovazan
Copy link
Member

And sure enough, the binary is not stripped properly. There should not be any S_P_CoreLib strings in a stripped binary, but there's lots of them. This is from #102183, but it's a benchmark setup issue; stripping doesn't do the right thing.

Thanks Michal for pointing to the root cause!
We are applying the same stripping technique for both NativeAOT and MonoAOT to have "fair" comparison stripping only the local symbols (via: strip -x

Utils.RunProcess(Logger, "strip", $"-no_code_signature_warning -x {appPath}/{filename}", workingDir: Path.GetDirectoryName(appPath));
) - this is a limitation with Mono (we have a tracking issue to improve this: #81530 so we can do full stripping).

That being said, in cases like this, we should check if the MAUI app also regressed (MAUI apps use full stripping):

  • if it did, then we most probably have an issue which we should investigate further
  • if it didn't, we should consider the regression with a grain of salt - limitation of how benchmark is setup

@matouskozak
Copy link
Member

matouskozak commented Jul 2, 2024

I took a look at our MAUI measurements and the regression for the range (84b3339...de6897b) containing this change is minimal (0.014 MB) + the MAUI version also changed dotnet/maui@562713c...13942bd so it is likely not even related.
image

@ivanpovazan
Copy link
Member

@matouskozak in that case I would suggest then closing this issue and link the problem we experienced here to #81530

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jul 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

3 participants