Skip to content

Call AsyncXMethodBuilder.Create prior to storing fields to elide double zeroing#45262

Merged
jaredpar merged 2 commits intodotnet:release/dev16.8-preview1from
benaadams:async-zero-init
Jun 29, 2020
Merged

Call AsyncXMethodBuilder.Create prior to storing fields to elide double zeroing#45262
jaredpar merged 2 commits intodotnet:release/dev16.8-preview1from
benaadams:async-zero-init

Conversation

@benaadams
Copy link
Member

@benaadams benaadams commented Jun 17, 2020 •
edited
Loading

Currently Rosyln zeros the the AsyncMethodBuilder field with .locals init and then assigns the result of .Create to that field.

When .Create is inlined it is also zeros; so the assignment is a second set of zeroing.

The Jit has an optimization where it will remove the second set of zeroing dotnet/runtime#36918 ("Optimization to remove redundant zero initializations"); however this only kicks in if there aren't any preceding non-zero assignments.

e.g.

G_M51821_IG01:
       push     rsi
       sub      rsp, 144
       vzeroupper                                ; zero
       xor      rax, rax                         ;  |
       mov      qword ptr [rsp+28H], rax         ;  |
       vxorps   xmm4, xmm4                       ;  |
       mov      rax, -96                         ;  |
       vmovdqa  xmmword ptr [rsp+rax+90H], xmm4  ;  |
       vmovdqa  xmmword ptr [rsp+rax+A0H], xmm4  ;  |
       vmovdqa  xmmword ptr [rsp+rax+B0H], xmm4  ;  |
       add      rax, 48                          ;  |
       jne      SHORT  -5 instr                  ;  V
       mov      rsi, rdx
						;; bbWeight=1    PerfScore 8.58
G_M51821_IG02:
       mov      gword ptr [rsp+28H], r8      ; save param ** blocks redundant zero elimination **
       xor      ecx, ecx                     ; re-zero
       vxorps   xmm0, xmm0                   ;    |
       vmovdqu  xmmword ptr [rsp+38H], xmm0  ;    |
       vmovdqu  xmmword ptr [rsp+48H], xmm0  ;    |
       mov      qword ptr [rsp+58H], rcx     ;    V
       mov      dword ptr [rsp+30H], -1
       lea      rcx, [rsp+28H]
       call     System.Runtime.CompilerServices.AsyncMethodBuilderCore:Start(byref)
       lea      rcx, bword ptr [rsp+38H]
       mov      rdx, rsi
       call     System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1[ReadResult][System.IO.Pipelines.ReadResult]:get_Task():System.Threading.Tasks.ValueTask`1[ReadResult]:this
       mov      rax, rsi

This PR moves the call to .Create() to the first step; prior to saving parameters to the fields so they do not block the redundant zero elimination optimization.

/cc @erozenfeld @stephentoub

From dotnet/runtime#2325 (comment)

Regarding your second example where we first assign to a gc field and then initialize the struct field: currently my optimization doesn't handle that. It's possible to extend it to handle that case but that would require keeping track of field initializations and may be a little tricky.

However, now the answer to your question

Or would changing it to move all the zeroing above that first assignment to non-zero (which would be a much more complex refactoring in Roslyn) help?

is yes.

If the struct field zero initialization was done before the first assignment, it would be eliminated by the new optimization.

Resolves #45295

@benaadams benaadams requested a review from a team as a code owner June 17, 2020 19:36
This was referenced Jun 18, 2020
Closed
Closed
@cston cston requested a review from a team June 23, 2020 17:31
@erozenfeld erozenfeld mentioned this pull request Jun 24, 2020
Merged
@benaadams
Copy link
Member Author

benaadams commented Jun 24, 2020

Additional improvements have been added to Jit to take more advantage of this Rosyln change dotnet/runtime#38314

@jaredpar
Copy link
Member

jaredpar commented Jun 25, 2020

Thanks for the update. I'm going to take a look at this later tonight.

@benaadams
Copy link
Member Author

benaadams commented Jun 27, 2020

It's quite a common pattern; especially since CancellationToken is normally one of the parameters even if there aren't any others

@jaredpar
Copy link
Member

jaredpar commented Jun 27, 2020

@benaadams thanks this looks great!

Can u retarget this to release/dev16.8-preview1? At the moment "master" targets 16.7 and we're pretty late in the cycle there. Really only taking significant bug fixes.

@benaadams benaadams changed the base branch from master to release/dev16.8-preview1 June 27, 2020 15:01
@benaadams
Copy link
Member Author

benaadams commented Jun 27, 2020

Can u retarget this to release/dev16.8-preview1?

Done

@jaredpar jaredpar merged commit db87fbd into dotnet:release/dev16.8-preview1 Jun 29, 2020
@jaredpar
Copy link
Member

jaredpar commented Jun 29, 2020

Thanks @benaadams!

@benaadams benaadams deleted the async-zero-init branch June 29, 2020 03:40
@benaadams benaadams mentioned this pull request Jun 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Order of initialization in async kickoff method with params blocks Jit zero init eliding

4 participants

@benaadams @jaredpar @cston @RikkiGibson