Call AsyncMethodBuilder.Start on field not on local by benaadams · Pull Request #41253 · dotnet/roslyn

benaadams · 2020-01-28T05:15:50Z

Removes an unnecessary struct copy to local of the AsyncMethodBuilder-like; and calls .Start on the mutable field and not the inert local copy.

As well as being more semantically correct (see #41222) helps most with AsyncValueTaskMethodBuilder<TResult> as they can be quite large.

e.g. the AsyncValueTaskMethodBuilder for System.IO.Pipelines ValueTask<ReadResult> at 48 bytes with 3 object references

Type layout for 'AsyncValueTaskMethodBuilder`1'
Size: 48 bytes. Paddings: 13 bytes (%27 of empty space)
|==========================================================|
|     0: Boolean _haveResult (1 byte)                      |
|----------------------------------------------------------|
|     1: Boolean _useBuilder (1 byte)                      |
|----------------------------------------------------------|
|   2-7: padding (6 bytes)                                 |
|----------------------------------------------------------|
|  8-15: AsyncTaskMethodBuilder`1 _methodBuilder (8 bytes) |
|----------------------------------------------------------|
| 16-47: ReadResult _result (32 bytes)                     |
| |====================================================|   |
| |     0: ResultFlags _resultFlags (1 byte)           |   |
| |----------------------------------------------------|   |
| |   1-7: padding (7 bytes)                           |   |
| |----------------------------------------------------|   |
| |  8-31: ReadOnlySequence`1 _resultBuffer (24 bytes) |   |
| | |======================================|           |   |
| | |   0-7: Object _startObject (8 bytes) |           |   |
| | |--------------------------------------|           |   |
| | |  8-15: Object _endObject (8 bytes)   |           |   |
| | |--------------------------------------|           |   |
| | | 16-19: Int32 _startInteger (4 bytes) |           |   |
| | |--------------------------------------|           |   |
| | | 20-23: Int32 _endInteger (4 bytes)   |           |   |
| | |======================================|           |   |
| |====================================================|   |
|==========================================================|

Resolves: #20606

mikedn · 2020-01-28T07:19:36Z

@benaadams Not sure why you requested my review, perhaps you were looking for someone else with a similar name or something?

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.cs

benaadams · 2020-01-28T07:21:49Z

Not sure why you requested my review

You raised issue "Unnecessary copy generated in async code" #21139 which this addresses?

benaadams · 2020-01-28T07:27:22Z

@mikedn mostly is the il change in the tests would you would expect for resolving "Unnecessary copy generated in async code" #21139

mikedn · 2020-01-28T07:31:01Z

LOL, I have no recollection of ever opening such an issue in the Roslyn repo. I'll try to take a look when a get home from work.

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/LocalSlotMappingTests.cs

mikedn · 2020-01-28T18:56:03Z

The generated IL looks correct to me. Not familiar with Roslyn so I can't comment on the code change itself.

jaredpar · 2020-01-28T18:56:45Z

@benaadams

Type layout for 'AsyncValueTaskMethodBuilder`1'

What tool are you using to produce that ASCII art diagram of type layout?

benaadams · 2020-01-28T19:02:59Z

What tool are you using to produce that ASCII art diagram of type layout?

@SergeyTeplyakov's https://github.com/SergeyTeplyakov/ObjectLayoutInspector (nuget)

benaadams · 2020-01-29T02:14:49Z

Not sure an easy way to insert the assignment of .Create() return to field (zero assigns), before the assignment of parameters to the fields as the parameters are assigned in caller (StateMachineRewriter.GenerateKickoffMethodBody) before its called.

So I guess this PR is as is

benaadams · 2020-01-29T02:32:12Z

Raised issue for Jit double zeroing dotnet/runtime#2325

benaadams · 2020-02-03T22:24:55Z

Converts

public async ValueTask<ReadResult> OuterMethod(object o)
{
    return await InnerMethod(o);

    static async ValueTask<ReadResult> InnerMethod(object o)
    {
        return default;
    }
}

From

.method public hidebysig 
	instance valuetype [System.Runtime]System.Threading.Tasks.ValueTask`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> OuterMethod (
		object o
	) cil managed 
{
	.custom instance void [System.Runtime]System.Runtime.CompilerServices.AsyncStateMachineAttribute::.ctor(class [System.Runtime]System.Type) = (
		01 00 19 50 72 6f 67 72 61 6d 2b 3c 4f 75 74 65
		72 4d 65 74 68 6f 64 3e 64 5f 5f 37 00 00
	)
	// Method begins at RVA 0x2050
	// Code size 57 (0x39)
	.maxstack 2
	.locals init (
		[0] valuetype Program/'<OuterMethod>d__7',
		[1] valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>
	)

	IL_0000: ldloca.s 0
	IL_0002: ldarg.1
	IL_0003: stfld object Program/'<OuterMethod>d__7'::o
	IL_0008: ldloca.s 0
	IL_000a: call valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<!0> valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::Create()
	IL_000f: stfld valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0014: ldloca.s 0
	IL_0016: ldc.i4.m1
	IL_0017: stfld int32 Program/'<OuterMethod>d__7'::'<>1__state'
	IL_001c: ldloc.0
	IL_001d: ldfld valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0022: stloc.1
	IL_0023: ldloca.s 1
	IL_0025: ldloca.s 0
	IL_0027: call instance void valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::Start<valuetype Program/'<OuterMethod>d__7'>(!!0&)
	IL_002c: ldloca.s 0
	IL_002e: ldflda valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0033: call instance valuetype [System.Runtime]System.Threading.Tasks.ValueTask`1<!0> valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::get_Task()
	IL_0038: ret
} // end of method Program::OuterMethod

To

.method public hidebysig 
	instance valuetype [System.Runtime]System.Threading.Tasks.ValueTask`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> OuterMethod (
		object o
	) cil managed 
{
	.custom instance void [System.Runtime]System.Runtime.CompilerServices.AsyncStateMachineAttribute::.ctor(class [System.Runtime]System.Type) = (
		01 00 19 50 72 6f 67 72 61 6d 2b 3c 4f 75 74 65
		72 4d 65 74 68 6f 64 3e 64 5f 5f 37 00 00
	)
	// Method begins at RVA 0x2050
	// Code size 55 (0x37)
	.maxstack 2
	.locals init (
		[0] valuetype Program/'<OuterMethod>d__7'
	)

	IL_0000: ldloca.s 0
	IL_0002: ldarg.1
	IL_0003: stfld object Program/'<OuterMethod>d__7'::o
	IL_0008: ldloca.s 0
	IL_000a: call valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<!0> valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::Create()
	IL_000f: stfld valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0014: ldloca.s 0
	IL_0016: ldc.i4.m1
	IL_0017: stfld int32 Program/'<OuterMethod>d__7'::'<>1__state'
	IL_001c: ldloca.s 0
	IL_001e: ldflda valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0023: ldloca.s 0
	IL_0025: call instance void valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::Start<valuetype Program/'<OuterMethod>d__7'>(!!0&)
	IL_002a: ldloca.s 0
	IL_002c: ldflda valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult> Program/'<OuterMethod>d__7'::'<>t__builder'
	IL_0031: call instance valuetype [System.Runtime]System.Threading.Tasks.ValueTask`1<!0> valuetype [System.Runtime]System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1<valuetype [System.IO.Pipelines]System.IO.Pipelines.ReadResult>::get_Task()
	IL_0036: ret
} // end of method Program::OuterMethod

cston

LGTM. Thanks @benaadams!

jaredpar · 2020-02-12T07:05:18Z

@stephentoub are you good with the proposed code gen changes here?

stephentoub · 2020-02-12T16:22:54Z

@stephentoub are you good with the proposed code gen changes here?

Seems fine. I can't think of anything this would negatively impact.

AlekseyTs · 2020-02-12T16:30:54Z

Are similar changes applicable to VB compiler?

cston · 2020-02-20T21:05:27Z

Are similar changes applicable to VB compiler?

It looks like VB calls Start with the state machine field - see AsyncRewriter.vb.

cston · 2020-02-20T21:38:12Z

@dotnet/roslyn-compiler for a second review, please.

cston · 2020-02-20T22:57:39Z

@benaadams, thanks for the contribution.

Call AsyncMethodBuilder.Start on field not on local

e22f035

benaadams requested a review from sharwell January 28, 2020 05:29

Fix some tests

c3fd8d7

benaadams force-pushed the Call-AsyncMethodBuilder.Start-on-field branch from 92a9a64 to c3fd8d7 Compare January 28, 2020 06:36

benaadams requested a review from mikedn January 28, 2020 06:37

benaadams marked this pull request as ready for review January 28, 2020 07:00

benaadams requested a review from a team as a code owner January 28, 2020 07:00

benaadams commented Jan 28, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.cs Show resolved Hide resolved

benaadams mentioned this pull request Jan 28, 2020

AsyncMethodBuilder.Start called with a different this reference than AsyncMethodBuilder.Task #41222

Closed

benaadams requested a review from stephentoub January 28, 2020 07:53

benaadams force-pushed the Call-AsyncMethodBuilder.Start-on-field branch from dd6b184 to c3fd8d7 Compare January 28, 2020 08:15

mikedn reviewed Jan 28, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/LocalSlotMappingTests.cs Show resolved Hide resolved

jcouv added Area-Compilers Feature - Async Streams Async Streams labels Jan 28, 2020

jcouv added Feature - ValueTask ValueTask and removed Feature - Async Streams Async Streams labels Jan 29, 2020

jcouv mentioned this pull request Jan 30, 2020

Async state machine 'Start' called on a copy of the state machine #20606

Closed

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 30, 2020

benaadams mentioned this pull request Feb 3, 2020

Jit double zeros dotnet/runtime#2325

Closed

cston approved these changes Feb 11, 2020

View reviewed changes

jaredpar approved these changes Feb 20, 2020

View reviewed changes

cston merged commit a3145c0 into dotnet:master Feb 20, 2020

benaadams deleted the Call-AsyncMethodBuilder.Start-on-field branch February 21, 2020 02:28

timcassell mentioned this pull request Oct 12, 2020

Async Method Builder Unexpected Results #48518

Closed

Conversation

benaadams commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Jan 28, 2020

Uh oh!

Uh oh!

benaadams commented Jan 28, 2020

Uh oh!

benaadams commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Jan 28, 2020

Uh oh!

Uh oh!

mikedn commented Jan 28, 2020

Uh oh!

jaredpar commented Jan 28, 2020

Uh oh!

benaadams commented Jan 28, 2020

Uh oh!

benaadams commented Jan 29, 2020

Uh oh!

benaadams commented Jan 29, 2020

Uh oh!

benaadams commented Feb 3, 2020

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 12, 2020

Uh oh!

stephentoub commented Feb 12, 2020

Uh oh!

AlekseyTs commented Feb 12, 2020

Uh oh!

cston commented Feb 20, 2020

Uh oh!

cston commented Feb 20, 2020

Uh oh!

cston commented Feb 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benaadams commented Jan 28, 2020 •

edited

Loading

benaadams commented Jan 28, 2020 •

edited

Loading