-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Blazor WASM] - Investigate 8% size regression in System.Memory.dll.br #51571
Comments
Tagging subscribers to this area: @GrabYourPitchforks, @carlossanlop Issue DetailsSize increase for System.Memory.dll.br as observed from https://msit.powerbi.com/groups/me/apps/54e0e83f-07bc-45bf-87b7-a7677ff3af2a/dashboards/fa051820-ff60-4d40-8a08-bdcc1b47b1d0 cc @CoffeeFlux
|
@SamMonoRT Do you have the raw files on disk for the before and after images? That would definitely help investigation, since we could look at the entire call graph and see if the regression was due to code within System.Memory or is due to changes in one of its callers. |
I don't have that information. I just saw an auto-filled issue by @DrewScoggins which has some more information : DrewScoggins/performance-2#5087 |
Do you need the trimmed files or the actual full binaries from the NETCore.App package? |
The files after trimming but before compression would be best. That way I can plug the whole mess through ilspy and see what changed. |
I am also going to make a change so that these artifacts are preserved by Helix, as reproducing them took me about 30 minutes. |
Sorry, I should clarify: I need all files, not just System.Memory.dll. Otherwise I can't go back in the call graph. |
My best guess is that this is a consequence of #51025. The JSON stack now has direct dependencies to runtime/src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.To.cs Lines 47 to 54 in ccc25a9
Specifically, the above code instantiates an // System.Buffers.ArrayBufferWriter<T> - BASELINE
using System;
using System.Buffers;
using System.Runtime.CompilerServices;
public sealed class ArrayBufferWriter<T> : IBufferWriter<T>
{
public ReadOnlyMemory<T> WrittenMemory
{
[MethodImpl(MethodImplOptions.NoInlining)]
get
{
throw new NotSupportedException("Linked away");
}
}
public ReadOnlySpan<T> WrittenSpan
{
[MethodImpl(MethodImplOptions.NoInlining)]
get
{
throw new NotSupportedException("Linked away");
}
}
public int WrittenCount
{
[MethodImpl(MethodImplOptions.NoInlining)]
get
{
throw new NotSupportedException("Linked away");
}
}
[MethodImpl(MethodImplOptions.NoInlining)]
public void Clear()
{
throw new NotSupportedException("Linked away");
}
[MethodImpl(MethodImplOptions.NoInlining)]
public void Advance(int count)
{
throw new NotSupportedException("Linked away");
}
[MethodImpl(MethodImplOptions.NoInlining)]
public Memory<T> GetMemory(int sizeHint = 0)
{
throw new NotSupportedException("Linked away");
}
} |
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsSize increase for System.Memory.dll.br as observed from https://msit.powerbi.com/groups/me/apps/54e0e83f-07bc-45bf-87b7-a7677ff3af2a/dashboards/fa051820-ff60-4d40-8a08-bdcc1b47b1d0 cc @CoffeeFlux
|
Related to #51311.
FWIW the node types can't be trimmed away today because it is rooted by these converters which are in turn rooted by this logic, which happens when existing cc @steveharter on thoughts on whether we can minimize the dependency on |
We can change the implementation to use |
Size increase for System.Memory.dll.br as observed from https://msit.powerbi.com/groups/me/apps/54e0e83f-07bc-45bf-87b7-a7677ff3af2a/dashboards/fa051820-ff60-4d40-8a08-bdcc1b47b1d0
cc @CoffeeFlux
The text was updated successfully, but these errors were encountered: