Proxy scenario perf regression

[sebastienros]

Actually in CoreFx, but opening in aspnet until I get more information

Updated

3.0.0-preview-27408-1 -> 3.0.0-preview-27409-1

Microsoft.NetCore.App / Core FX
dotnet/corefx@c2bbb6a...e1c0e1d

Microsoft.NetCore.App / Core CLR
dotnet/coreclr@a51af08...4e5df11

Command lines:

dotnet run -- --server http://10.10.10.2:5001 --client http://10.10.10.4:5002 `
-j ..\Benchmarks\benchmarks.httpclient.json -n HttpClientFactory --sdk Latest --self-contained --aspnetcoreversion 3.0.0-preview3-19116-01 --runtimeversion 3.0.0-preview-27408-1

dotnet run -- --server http://10.10.10.2:5001 --client http://10.10.10.4:5002 `
-j ..\Benchmarks\benchmarks.httpclient.json -n HttpClientFactory --sdk Latest --self-contained --aspnetcoreversion 3.0.0-preview3-19116-01 --runtimeversion 3.0.0-preview-27409-1

[benaadams]

Looks like two drops?

[sebastienros]

The first drop corresponds to when I changed the benchmarks to actually flow the headers.

[sebastienros]

@stephentoub this benchmark is mostly about HttpClient under load, no SSL. Do you see anything in the changes I linked to that could explain the regression?

[stephentoub]

Do you see anything in the changes I linked to that could explain the regression?

Nothing jumps out at me.

[sebastienros]

I will investigate then.

[sebastienros]

After investigating, it looks like a regression between

3.0.0-preview-27408-1 -> 3.0.0-preview-27408-3

Microsoft.NetCore.App / Core FX
dotnet/corefx@c2bbb6a...ede577c

Microsoft.NetCore.App / Core CLR
dotnet/coreclr@a51af08...4e5df11

Then I fetched both runtimes and tried to find which changed assemblies might cause this, and I can recover the previous performance by using System.Globalization.Native.so from the previous runtime.

Looking at the diff, there is this file that has changed:

src/corefx/System.Globalization.Native/pal_collation.c

Something I need to mention also is that after this runtime version is taken into account only this benchmarks is changed. Everything seems stable.

Here is the code that the benchmark is running:

https://github.com/aspnet/Benchmarks/blob/711f2b81cbd5dc1c2022cae0606e76a7eb79edc1/src/Proxy/Program.cs#L134-L144

[sebastienros]

/cc @filipnavara @janvorli as it seems to come from this commit: https://github.com/dotnet/coreclr/pull/22390/files

I'd say that it's not because of this PR dotnet/coreclr@34d50b0 though they are pretty close but the diff starts on Feb 04. To be sure I could run a benchmark if someone could give me the System.Gloablization.Native.so that only reverts this commit.

[filipnavara]

Are you absolutely positive about those commit lists? I'd expect dotnet/coreclr#22378 to be the primary suspect if System.Globalization.Native is involved. It's just one commit prior to the lists you linked. The change in https://github.com/dotnet/coreclr/pull/22390/files should be harmless unless the C runtime glibc library is significantly broken.

I'll try to reproduce the results on the weekend, but if you find anything out please share it here.

[filipnavara]

I'm going through possible regressions from dotnet/coreclr#22378.

This line is missing customRules->size == 0 condition. Some callocs could be replaced with mallocs and avoid zeroing memory. There's potential for some performance regression in some of the *SortHandle functions, but not sure if it's big enough to account for this. I will submit patch to address the obvious things.

[janvorli]

I have applied a delta to the current master that undoes the change made in dotnet/coreclr#22390 and shared the System.Globalization.Native.so with @sebastienros. He has verified that it fixes the perf regression.

[sebastienros]

@filipnavara if you are not sure whether a change is beneficial or not, just build many versions and I can run the numbers for each build. Put them on //scratch2.

[filipnavara]

@janvorli Wow. That's very surprising. I'll look at it again. The change itself doesn't fix any bug and it should have generally improved performance which makes it surprising that quite the opposite happened. If I don't come up with a good explanation I'll submit a PR to revert it.

[sebastienros]

I am quite confident with the way I ran it, but there is always space for a human error (me). To be very confident feel free to give me versions with obvious regressions, even based on the reverted commit, to be sure I get the expected results. That would also make me fell more confident.

Something I want to insist on also is that I just patch "this" file, no other one, and I am not sure if one or the other versions could actually trigger some behavior coming from a separate module. I can also share the full standalone application if you want to check which bits are measured. But you will see that only this file will have changed in the end.