8.4 MB size regression updating System.Text.Json to 8.0.0 #84922
Comments
These AOT binary size footprint improvements assume that the reflection-based fallback is trimmed. Is the reflection-based fallback trimmed in your setup? |
We have trimming enabled for the whole app, with just these 4 directives for System.Text.Json: <Type Name="System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.DictionaryOfTKeyTValueConverter`3" Activate="Required Public" />
<Type Name="System.Text.Json.Serialization.Converters.ListOfTConverter`2" Activate="Required Public" />
<Type Name="MicrosoftStore.SomeNamespace.SomeOtherCustomConverterType" Activate="Required Public" />Which are needed to work around #78029. These have been in place since we started migrating to System.Text.Json though, so they're not new. I guess I could also try removing these just as a sanity check, but I'd really be surprised if these had anything to do with the issue here. What I find confusing is that the .NET Native linker is generally quite aggressive with trimming, so such a big increase is unexpected. It seems 8.0 is somehow causing a whole lot more (like, a lot more) stuff to get rooted 🤔 I've kicked off a couple builds with EDIT: if we don't come up with any other ideas and that SizeBench tool doesn't help, one option could also be for me to try cherry-picking #84899 into the various release branches for the .NET 8 Previews, build them and then check the size with each of them? At least that would help us narrow down when exactly things went wrong? |
|
Is the code under https://github.com/dotnet/runtime/search?q=IsReflectionEnabledByDefault getting trimmed in your app? |
|
Mmh pretty sure .NET Native doesn't handle |
|
Size of reflection engine is likely to grow over time as more features get added to it. I may explain at least some of the size growth that you are seeing.
Yes, it would be useful to collect the data on where the growth is coming from. |
|
I'm struggling to get SizeBench working, do you know if it actually supports .NET Native binaries? 🤔
Would you by any chance know if there's a way to specify this mode from an MSBuild property or something? If not, Anton suggested building locally with |
|
IIRC, x64 binaries are not produced by link.exe in UWP toolchain. They are produced by mdil binder that is apparently missing some of the details that SizeBench expects. You can try building and comparing arm64 binaries. Arm64 is produced by link.exe and so you may have better luck with them. |
|
Ah, right, I forgot the Arm64 linker was different than the x64 one 😄 Stack trace (click to expand):I take it SizeBench doesn't really support .NET Native binaries after all. Anton mentioned there might be some ilc.exe options to dump the dependenty graph, I'll see if I can find someone that remembers how to use that. Maybe @MichalStrehovsky? Otherwise I can also ask Tom once he's back 🙂 |
|
I responded on Teams but I looked at the exception you're hitting now and have a separate idea. SizeBench works fine with NativeAOT and it's not that different. SizeBench is open source. If you're determined enough, you could probably build it from source and avoid this exception: By replacing it with return. The method does a return for MASM or clang a couple lines above, so it should be fine to just return from here and still get usable results. |
|
I've managed to build and run SizeBench locally with that fix, thank you! 😄
Also loading the symbols diff for that, but it's been loading for over 3 hours now and still 80% through 😆 EDIT: it finished! And I managed to export all diff table to excel: here's a direct link (need a MSFT account to open this). @jkotas @MichalStrehovsky would you be able to take a look and see if anything in particular jumps to your eyes here, whenever you have a spare minute? You're definitely way more familiar with this kind of data than I am 😅
EDIT2: thanks to @tannergooding for pointing this out, it seems the deltas in the diff are just "inverted", ie. with STJ7 being the baseline, so the size increase is actually a negative value in the spreadsheet. If you sort the diffs by lower to higher, and sum all the negative diffs, you get ~4.5MB of size regression. If you then add that ~1.2MB improvement, you end up with that ~3.4 MB regression for the Arm64 binary, which seems to add up.
In the meantime, I've also started to gather some data points by cherry-picking Jan's fix in #84899 into the various 8.0.0 Preview 1, 2, 3 branches, and kicking off new pipelines to try to narrow down the range of changes that introduced this regression. So far the first two pipelines have finished, and it seems the regression is already there since Preview 1. Specifically:
This is actually ~350 KB worse than with 8.0.0-preview.4.23216.5, I guess some of the new changes there (such as #84411) did improve things a little bit at least. It would seem the culprit then is somewhere in v8.0.0-preview.1.23110.8...v7.0.2. It seems there's a total of 461 commits, though most are not about System.Text.Json changes, so with some luck it should be possible to narrow this down to some specific change that look suspicious? I can then build from there and calculate new diffs to validate. EDIT: actually I'm not 100% sure that commits delta is right, some things don't really add up 🤔 For additional context, here's the size diff for the uncompressed binaries extracted from the package:
As expected, the difference is even more noticeable here. The x64 binary in particular is 17.4% bigger. Does anyone have any ideas, maybe @eiriktsarpalis? Also could we add the |
|
Sharing more details as I keep investigating this. Will update this comment with more data as I go 😄 Looking at the SizeBench diffs (link here), there seem to be these main offenders showing up in the top results:
I'm still not entirely sure how to check diffs, as the commits range seems to be broken in the runtime (@tannergooding mentioned it's possibly due to the repo having more than one tree). Additionally, some of the types I'm seeing in the diff (such as
Which are not showing up in that commits range though, so not entirely sure what's up with the git history here (as in, those PRs are from July/August 2023, so they should be there..?). It just seems some change is now causing a whole lot of additional (and useless) reflection stuff to be rooted even though you're only ever using the source generators, such in our case. cc. @eiriktsarpalis @krwq as you've both worked on those various PRs 🙂 EDIT: actually these might be fine, as the issue was introduced after November 23rd, 2023, see below. EDIT 2: this one from November 25th is suspect, looking into it. EDIT 3: well it seems that's not it, I built 22575 right from that commit and the binary size is fine (see table below). I'm still investigating and running pipelines à la git bisect to try to narrow down what PR exactly introduced the issue. I'm hoping once we figure that out, we might come up with a way to shuffle the code a bit to stop rooting stuff downlevel. Here's what I have so far:
It seems the issue is somewhere between 8.0.0-alpha.1.22570.1 and 8.0.0-preview1. Unfortunately that range is after #78741, so I'll have to keep cherry-picking #84899 manually and build from source, so it'll take a bit longer to gather more results. But, working on it. This does seem very promising so far, we're narrowing this down a lot 🎉 EDIT: 8.0.0-alpha.23053 (built from Sergio0694@5f8fc67) is very interesting. The binary size is actually even worse here than it is in Preview 1. It seems there's been some change that severely regressed the size between 22575 (built from Sergio0694@58f59db) and 23053, which then improved again by the time 8.0.0 Preview 1 came out, but never really managed to get back to the previous baseline. Definitely seems like we're narrowing down on what is causing this though. EDIT 2: it seems the break is between 8.0.0-alpha.22605 (built from Sergio0694@d47a24c) and 8.0.0-alpha.22615 (built from Sergio0694@a46dd51). These two seem suspect and I'm inclined to say one of the two is likely the culprit here: |
Sergio0694
changed the title
layomia
added
the
partner-impact
|
Well, this was not painful at all, but we did narrow down exactly which PRs regressed the size! 🎉🎉🎉 See table in the previous message for full breakdown and deltas between a whole lot of commits. Specifically:
That is, these two PRs seem to be causing the regressions:
@jkotas @MichalStrehovsky do you see anything in particular that jumps to your eyes and might explain the very noticeable worse binary size in the changes for those two PRs? Especially in the second one. I will also try to spend some time tomorrow going through the changes in those two PRs in particular (haven't had time to do that just yet). Still, this seems like a very good first step trying to figure out what's going on and hopefully how to fix it 😄 |
danmoseley
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue DetailsDescriptionNow that #84895 has been fixed, I was able to start dogfooding System.Text.Json 8.0.0 builds in the Microsoft Store, to gather some numbers so we can more easily track the binary size difference with this new release. This is where I've noticed an problem: my understanding is that the new 8.0.0 release is expected to include several binary size improvements, which should make the update result in a smaller binary size footprint (see #79122), but I'm actually seeing a regression, and not even a trivial one 🥲 Configuration
Delta: +3.605 KB (+5.4%). The real delta for the x64 binary is actually ~8.4 MB (ie. ~17%), see details below. Regression?Yes. The binary size is actually, surprisingly, lower on 7.0.2. I assume there's definitely something wrong going on here? Happy to help test any preview bits necessary and/or to provide more info (internally if needed, as the project is not public).
|
I do not see anything obvious. |
|
Can https://github.com/MichalStrehovsky/sizoscope be used to investigate here? There is a diff'ing capability in it. |
|
Unfortunately that's only for NativeAOT and doesn't support .NET Native, which is what the Microsoft Store uses 🥲 As suggested by @eiriktsarpalis though, I can try seeing if I can create a standalone repro for this. If I can keep all code for the repro in just a .NET Standard 2.0 lib, we could then compile that both with .NET Native and NativeAOT. Assuming the regression also repros on NativeAOT, we'd then be able to use sizoscope there. If not, well, worth a try, and it might still provide us with more useful info to further investigate this. If anything else, it'd make testing things much faster than building the Store 😄 |
|
I have some new results with the latest nightly, as @eiriktsarpalis asked to re-run SizeBench with it as well.
Deltas:
Full SizeBench diff: click here (MSFT internal only). Also trying to get some trimming/reflection logs from ILC in the meantime. |
|
I'm seeing the |
|
Alright I have lots more (hopefully) useful info 😄
Same if I use any of the This is all that's needed to reproduce this, in a blank UWP app with a System.Text.Json dependency: Repro code (click to expand):public sealed partial class MainPage : Page
{
public MainPage()
{
this.InitializeComponent();
ProcessData(JsonSerializer.Serialize(new MyModel(), MyJsonSerializerContext.Default.Options));
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static void ProcessData(object jsonModel)
{
Console.WriteLine(jsonModel.ToString());
Trace.WriteLine(jsonModel.ToString());
}
}
public sealed class MyModel
{
public string Foo { get; set; }
public int Bar { get; set; }
}
[JsonSerializable(typeof(MyModel))]
[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
public sealed partial class MyJsonSerializerContext : JsonSerializerContext
{
}Here's the full source code for the repro project: JsonBinarySizeRepro.zip.
A few notable things I've noticed looking at the ILTransformed files, check out these diffs:
I'm hoping this minimal repro and also having all the ILC logs might allow us to figure out what's exactly rooting stuff. I might be wrong but it seems like there's something in 8.0.0 that's causing the trimmer to assume that a whole lot of System.Text.Json stuff will be accessed through reflection, which then causes it to preserve a ton more stuff compared to 7.0.2, along with all metadata and also compiled code for all those bits? But like... Why? 🥲 @jkotas @MichalStrehovsky whenever you have some time, would you be able to take a look at those intermediate files from the ILC dumps to see if you notice anything that makes sense to you to explain what's going on? It's all in that .zip link, and I've also linked the CreateMetadataInput.log and reflectionlog.csv files from both versions directly in the list above for convenience. Thank you! |
MCG scans for everything that may need WinRT marshalling and generates the marshaller for everything that it can find. For example, implementing |
|
I am not able to tell from these logs where things are rooted. However, there seems to be infinite generic recursion that involves collection types. The logs have types like:
|
Most likely related to this method: However this would only be rooted by a call to one of the |
|
Here is a small repro with regular native AOT with .NET 8 P3: using System.Text.Json;
using System.Text.Json.Serialization.Metadata;
JsonSerializer.DeserializeAsyncEnumerable<Queue<string>>(new MemoryStream(), (JsonTypeInfo<Queue<string>>)null!);.csproj: Publishing with |
|
@eiriktsarpalis I'm wondering whether that instance field It's interesting though that NativeAOT also has some kind of issues in that area, though it seems in order to trigger them you need to explicitly use that Would it be possible to eg. change that instance field to just be a non-generic version, and then just directly cast from within that method when needed? Ie. just have the field be a If it helps, I guess I can also try wiggling that code, then build from source and try out that package with the repro project. |
You do not have to use Queue explicitly. It repros with any reference type. For example, this will produce the warning with native AOT as well: |
|
This definitely seems like it could explain the massive binary size increase on .NET Native.
|
|
@eiriktsarpalis Could you please look into this? It is complicated generic cycle that is caused by special-casing of |
|
Curious - now that we likely have identified the cause of the regression, do the changes in #78646 and #79397 make more sense as to how they managed to trigger the issue while looking just fine on their own? Or is it just more of a coincidence that they just happened to exacerbate the underlying issue of that generic recursion that was already there from previous PRs? 🤔 |
Yes, #78646 introduced the generic cycle. #79397 added more code that gets multiplied by the generic cycle before the compiler decided to cut it off and print warning. |
|
FWIW the dependency on |
ghost
added
the
in-pr
|
Results in the Store with a new build from #85176 (f10bc02):
Deltas:
We made it!! 🎉🎉🎉 |
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
Description
Now that #84895 has been fixed, I was able to start dogfooding System.Text.Json 8.0.0 builds in the Microsoft Store, to gather some numbers so we can more easily track the binary size difference with this new release. This is where I've noticed an problem: my understanding is that the new 8.0.0 release is expected to include several binary size improvements, which should make the update result in a smaller binary size footprint (see #79122), but I'm actually seeing a regression, and not even a trivial one 🥲
Configuration
Delta: +3.605 KB (+5.4%). The real delta for the x64 binary is actually ~8.4 MB (ie. ~17%), see details below.
Regression?
Yes. The binary size is actually, surprisingly, lower on 7.0.2.
cc. @eiriktsarpalis @layomia
I assume there's definitely something wrong going on here?
Ie. my assumption would be that updating to 8.0.0 should definitely result in a net decrease of binary size, no? 🤔
Happy to help test any preview bits necessary and/or to provide more info (internally if needed, as the project is not public).
The text was updated successfully, but these errors were encountered: