Pipelines remove lock from uncontended path

[benaadams]

Simpler version of #36955

Drops the lock for common path through the highlighted code paths for Json TE:

Before

After

Should follow up with more state out to the PipeOperationState to avoid cache thrashing, but should be easier to review.

/cc @halter73 @adamsitnik @stephentoub @davidfowl

[benaadams]

System.IO.Pipelines.zip

[adamsitnik]

[benaadams]

Interesting, did you managed to get a trace? Json would be interesting to see how things shifted around

[benaadams]

Ah, let me add cache seperation

[adamsitnik]

Interesting, did you managed to get a trace?

@benaadams I've just uploaded PlatformJson trace to traces/36956_pipes_locks

[benaadams]

Does look to behave as expected

Before

After (still lock taken in GetMemory, so some contention there)

[benaadams]

The GetMemory() showing up as still taking the lock (although much less) means there is contention on pipe between reader and writer which shouldn't happen here for Json.

It looks like _waitForData == false so the FlushAsync() happens triggering the reader and then the writer immediately calls GetMemory() causing contention; the fix is to call GetMemory() prior to calling FlushAsync() in this scenario dotnet/aspnetcore#22201

[davidfowl]

@adamsitnik can you collect counters before and after the next run. I want to make sure we’re looking at the right metrics

[benaadams]

Merged the commits so can try something more easily

[adamsitnik]

can you collect counters before and after the next run

@davidfowl I've re-run the benchmarks against latest changes and included the Lock Contention (#/s) metric.

Machine	Benchmark	RPS before	RPS after	locks/s before	locks/s after
Citrine 28 cores	PlaintextPlatform	9,199,114	9,379,908	401	505
	JsonPlatform	1,202,437	1,185,605	20	32
	FortunesPlatform	313,169	315,329	81	65
	Fortunes Batching	425,197	414,649	651	631
Perf 12 cores	PlaintextPlatform	5,997,033	5,940,848	114	98
	JsonPlatform	605,564	607,849	13	10
	FortunesPlatform	130,403	129,567	65	68
ARM 32 cores	PlaintextPlatform	6,132,864	6,082,789	330	682
	JsonPlatform	566,019	578,091	310	152
	FortunesPlatform	96,260	91,606	19	29
Mono 56 cores	PlaintextPlatform	6,945,170	6,947,873	5,891	5,883
	JsonPlatform	1,113,376	1,149,895	995	666
AMD 46 cores	JsonPlatform	7,607,998	7,627,253	531	498
	JsonPlatform	719,018	675,684	68	61
	FortunesPlatform	260,262	258,619	18	20

the lock contention on Mono machine could most probably lead us to answer why it's not faster than Citrine (cc @stephentoub)

[davidfowl]

@adamsitnik are we comparing a fixed number of requests using bombardier? Or are we using wrk?

[adamsitnik]

are we comparing a fixed number of requests using bombardier? Or are we using wrk?

@davidfowl I am not sure whether bombardier or wrk are used, I am using the following arguments for BenchmarksDriver

--connections 512 --self-contained --sdk 5.0.100-preview.6.20272.2 --runtime  5.0.0-preview.6.20272.2 --aspnetcoreversion 5.0.0-preview.5.20255.6

[benaadams]

the lock contention on Mono machine could most probably lead us to answer why it's not faster than Citrine

Lock contention is reported after any spinning is been done and when it's going to sleep; rather than just raw contention that is resolved after spinning: #37023 (comment)

[halter73]

If you're using version 1 of the benchmark driver and you haven't specified a --job or --client-name, you're using wrk.

[benaadams]

Revist post 5.0