Assign (generic) struct to default causes overzealous CORINFO_HELP_ASSIGN_BYREF #12060
Labels
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Comments
benaadams
changed the title
|
.csproj <Project Sdk="Microsoft.NET.Sdk.Web">
<PropertyGroup>
<TargetFramework>netcoreapp3.0</TargetFramework>
<LangVersion>latest</LangVersion>
<TieredCompilation>false</TieredCompilation>
</PropertyGroup>
</Project>Program.cs using System.IO;
using System.Net;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
public class Startup
{
private static readonly byte[] _helloWorldBytes = Encoding.UTF8.GetBytes("Hello, World!");
public void Configure(IApplicationBuilder app)
{
app.Run((httpContext) =>
{
var response = httpContext.Response;
response.StatusCode = 200;
response.ContentType = "text/plain";
var helloWorld = _helloWorldBytes;
response.ContentLength = helloWorld.Length;
return response.Body.WriteAsync(helloWorld, 0, helloWorld.Length);
});
}
public static Task Main(string[] args)
{
var host = new WebHostBuilder()
.UseKestrel(options =>
{
options.Listen(IPAddress.Loopback, 5001);
})
.UseContentRoot(Directory.GetCurrentDirectory())
.UseStartup<Startup>()
.Build();
return host.RunAsync();
}
}Dasm filter Make 2 HTTP requests to First will inline |
|
il is |
|
cc @CarolEidt |
|
As a counter point public void Uninitialize()
{
_features = default;
_request.Uninitialize();
_response.Uninitialize();
_connection?.Uninitialize();
_websockets?.Uninitialize();
}It produces ; Assembly listing for method DefaultHttpContext:Uninitialize():this
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
; V00 this [V00,T00] ( 7, 7 ) ref -> rcx this class-hnd
;# V01 OutArgs [V01 ] ( 1, 1 ) lclBlk ( 0) [rsp+0x00] "OutgoingArgSpace"
; V02 tmp1 [V02,T03] ( 2, 4 ) ref -> rax class-hnd "dup spill"
; V03 tmp2 [V03,T05] ( 3, 2.50) ref -> rax
; V04 tmp3 [V04,T04] ( 2, 4 ) ref -> rax class-hnd "dup spill"
; V05 tmp4 [V05,T06] ( 3, 2.50) ref -> rax
; V06 tmp5 [V06,T01] ( 3, 6 ) ref -> rax class-hnd "Inlining Arg"
; V07 tmp6 [V07,T02] ( 3, 6 ) ref -> rax class-hnd "Inlining Arg"
;
; Lcl frame size = 0
G_M29359_IG01:
C5F877 vzeroupper
6690 nop
G_M29359_IG02:
488D4138 lea rax, bword ptr [rcx+56]
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
C5FA7F4020 vmovdqu qword ptr [rax+32], xmm0
C5FA7F4030 vmovdqu qword ptr [rax+48], xmm0
488B4108 mov rax, gword ptr [rcx+8]
8B10 mov edx, dword ptr [rax]
4883C018 add rax, 24
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
C5FA7F4020 vmovdqu qword ptr [rax+32], xmm0
C5FA7F4030 vmovdqu qword ptr [rax+48], xmm0
488B4110 mov rax, gword ptr [rcx+16]
8B10 mov edx, dword ptr [rax]
4883C010 add rax, 16
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
C5FA7F4020 vmovdqu qword ptr [rax+32], xmm0
488B4118 mov rax, gword ptr [rcx+24]
4885C0 test rax, rax
7422 je SHORT G_M29363_IG04
G_M29363_IG03:
4883C008 add rax, 8
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
G_M29365_IG04:
488B4120 mov rax, gword ptr [rcx+32]
4885C0 test rax, rax
7501 jne SHORT G_M29365_IG06
G_M29365_IG05:
C3 ret
G_M29365_IG06:
4883C008 add rax, 8
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
G_M29365_IG07:
C3 ret
; Total bytes of code 147, prolog size 5 for method DefaultHttpContext:Uninitialize():thisWork around might be to set to default, then initalize the 2 other items via method rather than using the struct .ctor? |
|
Workaround would be dotnet/aspnetcore#7659; however would also be an api breaking change so not sure it would be acceptable |
|
Which would mean I'd identified the wrong cause; as it would be Might be by design then? |
|
Right, constructors tend to not work well for big structs by design. |
|
And if I add second method |
|
The upstream change produces far better asm (as also don't have to set to So the question is, should this be closed ("by design") or left open as something that could be improved post inline (recognising the fields are null assigns)? As it only becomes a potential optimization after inlining the .ctor (which is why it shows in the asm); without inlining the .ctor it couldn't be determined that the copy could use lighter assignments. |
|
Closing, looks like its covered by this issue? https://github.com/dotnet/coreclr/issues/12812 |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Assigning references to null shouldn't need
Jit_ByRefWriteBarrierhowever assigning a struct with GC references to default causes lots of write barrier calls; whereas I'd have thought it would just assign them to null?e.g. in ASP.NET Core there is a feature cache that gets set to default in Initialize
FeatureReferences
DefaultHttpContext
What it seems to do is blank out enough stack; then assign via
CORINFO_HELP_ASSIGN_BYREFThis shows up at around 2% of total run time in the TE benchmarks:
/cc @AndyAyersMS @davidfowl @jkotas @mikedn
The text was updated successfully, but these errors were encountered: