Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine two table lookups into one while formatting Guid on Arm64 #87126

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

SwapnilGaikwad
Copy link
Contributor

Contributes towards the issue #84328

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 5, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2023
@SwapnilGaikwad
Copy link
Contributor Author

Micro-benchmarking on an N1 system shows the following improvements.

|                     Method |        Job |                                                                                             Toolchain |                        value |       Mean |     Error |    StdDev |     Median |        Min |        Max | Ratio | MannWhitney(2%) | Allocated | Alloc Ratio |
|--------------------------- |----------- |------------------------------------------------------------------------------------------------------ |----------------------------- |-----------:|----------:|----------:|-----------:|-----------:|-----------:|------:|---------------- |----------:|------------:|
|            FormatterDouble | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |     -1.7976931348623157E+308 | 719.049 ns | 1.7385 ns | 1.6262 ns | 719.644 ns | 714.862 ns | 720.304 ns |  1.00 |            Base |         - |          NA |
|            FormatterDouble | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |     -1.7976931348623157E+308 | 653.947 ns | 3.7146 ns | 3.4746 ns | 655.705 ns | 644.141 ns | 656.125 ns |  0.91 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                  -2147483648 |  27.632 ns | 0.0369 ns | 0.0327 ns |  27.623 ns |  27.583 ns |  27.693 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                  -2147483648 |  18.398 ns | 0.0239 ns | 0.0212 ns |  18.388 ns |  18.378 ns |  18.439 ns |  0.67 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |         -9223372036854775808 |  43.181 ns | 0.0768 ns | 0.0719 ns |  43.186 ns |  42.986 ns |  43.264 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |         -9223372036854775808 |  35.830 ns | 0.0053 ns | 0.0047 ns |  35.829 ns |  35.824 ns |  35.841 ns |  0.83 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            0 |  12.813 ns | 0.0066 ns | 0.0061 ns |  12.812 ns |  12.806 ns |  12.825 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            0 |   6.148 ns | 0.0020 ns | 0.0018 ns |   6.147 ns |   6.145 ns |   6.151 ns |  0.48 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            0 |   4.992 ns | 0.0092 ns | 0.0072 ns |   4.989 ns |   4.988 ns |   5.014 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            0 |   4.477 ns | 0.0014 ns | 0.0013 ns |   4.477 ns |   4.474 ns |   4.478 ns |  0.90 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterDouble | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |      1.7976931348623157E+308 | 717.626 ns | 2.3723 ns | 2.2190 ns | 718.797 ns | 714.878 ns | 720.597 ns |  1.00 |            Base |         - |          NA |
|            FormatterDouble | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |      1.7976931348623157E+308 | 656.692 ns | 3.8938 ns | 3.6422 ns | 658.688 ns | 646.814 ns | 659.652 ns |  0.92 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
| FormatterDateTimeOffsetNow | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun | 12/30/2017 3:45:22 AM -08:00 |  78.494 ns | 0.2743 ns | 0.2566 ns |  78.541 ns |  78.050 ns |  78.838 ns |  1.00 |            Base |         - |          NA |
| FormatterDateTimeOffsetNow | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun | 12/30/2017 3:45:22 AM -08:00 |  43.786 ns | 0.0035 ns | 0.0031 ns |  43.786 ns |  43.780 ns |  43.791 ns |  0.56 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   9.508 ns | 0.0012 ns | 0.0012 ns |   9.508 ns |   9.507 ns |   9.510 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   9.837 ns | 0.0018 ns | 0.0017 ns |   9.837 ns |   9.835 ns |   9.840 ns |  1.03 |          Slower |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |  17.086 ns | 0.0298 ns | 0.0279 ns |  17.100 ns |  17.033 ns |  17.116 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   9.658 ns | 0.0018 ns | 0.0016 ns |   9.658 ns |   9.657 ns |   9.661 ns |  0.57 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            4 |  13.620 ns | 0.1192 ns | 0.1115 ns |  13.627 ns |  13.457 ns |  13.817 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                            4 |   6.292 ns | 0.0005 ns | 0.0004 ns |   6.292 ns |   6.291 ns |   6.293 ns |  0.46 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |  12.839 ns | 0.0083 ns | 0.0077 ns |  12.837 ns |  12.825 ns |  12.852 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   8.271 ns | 0.0013 ns | 0.0011 ns |   8.271 ns |   8.269 ns |   8.273 ns |  0.64 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   6.370 ns | 0.0021 ns | 0.0019 ns |   6.370 ns |   6.368 ns |   6.374 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 |   5.455 ns | 0.0013 ns | 0.0012 ns |   5.455 ns |   5.452 ns |   5.456 ns |  0.86 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterDouble | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 | 308.785 ns | 0.4602 ns | 0.4304 ns | 308.800 ns | 308.037 ns | 309.567 ns |  1.00 |            Base |         - |          NA |
|            FormatterDouble | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                        12345 | 289.306 ns | 0.2112 ns | 0.1976 ns | 289.341 ns | 288.920 ns | 289.701 ns |  0.94 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|           FormatterDecimal | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   123456.789 | 180.820 ns | 0.3125 ns | 0.2923 ns | 180.794 ns | 180.095 ns | 181.339 ns |  1.00 |            Base |         - |          NA |
|           FormatterDecimal | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   123456.789 | 170.822 ns | 1.6344 ns | 1.5288 ns | 170.302 ns | 168.708 ns | 173.102 ns |  0.94 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |         18446744073709551615 |  38.537 ns | 0.0164 ns | 0.0146 ns |  38.537 ns |  38.509 ns |  38.561 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |         18446744073709551615 |  30.125 ns | 0.5294 ns | 0.4952 ns |  29.936 ns |  29.927 ns |  31.795 ns |  0.78 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   2147483647 |  17.946 ns | 0.0300 ns | 0.0266 ns |  17.941 ns |  17.907 ns |  17.994 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   2147483647 |  13.035 ns | 0.0020 ns | 0.0017 ns |  13.034 ns |  13.033 ns |  13.038 ns |  0.73 |          Faster |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|            FormatterUInt32 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   4294967295 |  11.676 ns | 0.0020 ns | 0.0019 ns |  11.676 ns |  11.673 ns |  11.679 ns |  1.00 |            Base |         - |          NA |
|            FormatterUInt32 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |                   4294967295 |  11.613 ns | 0.0094 ns | 0.0083 ns |  11.610 ns |  11.605 ns |  11.626 ns |  0.99 |            Same |         - |          NA |
|                            |            |                                                                                                       |                              |            |           |           |            |            |            |       |                 |           |             |
|             FormatterInt64 | Job-KYVZJY |    /main/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |          9223372036854775807 |  30.190 ns | 0.0034 ns | 0.0031 ns |  30.190 ns |  30.187 ns |  30.197 ns |  1.00 |            Base |         - |          NA |
|             FormatterInt64 | Job-RTINGX | /runtime/artifacts/bin/testhost/net8.0-linux-Release-arm64/shared/Microsoft.NETCore.App/8.0.0/corerun |          9223372036854775807 |  29.670 ns | 0.0136 ns | 0.0114 ns |  29.670 ns |  29.640 ns |  29.687 ns |  0.98 |            Same |         - |          NA |

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although, I'd prefer us doing this in JIT (to merge two subsequent table lookups)

@kunalspathak
Copy link
Member

/azp run runtime-coreclr libraries-jitstressregs, runtime-coreclr libraries-jitstress-random, runtime-coreclr libraries-jitstress, runtime-coreclr libraries-jitstress2-jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@kunalspathak
Copy link
Member

/azp run runtime-coreclr libraries-jitstressregs, runtime-coreclr libraries-jitstress-random, runtime-coreclr libraries-jitstress, runtime-coreclr libraries-jitstress2-jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo EgorBo added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 5, 2023
@ghost
Copy link

ghost commented Jun 5, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes towards the issue #84328

Author: SwapnilGaikwad
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SwapnilGaikwad
Copy link
Contributor Author

Seems like the test failures are because of the cancellations. Not sure why the tests got cancelled.

@kunalspathak
Copy link
Member

/azp run runtime-coreclr libraries-jitstressregs, runtime-coreclr libraries-jitstress-random, runtime-coreclr libraries-jitstress, runtime-coreclr libraries-jitstress2-jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@kunalspathak
Copy link
Member

/azp run runtime-coreclr libraries-jitstressregs, runtime-coreclr libraries-jitstress-random, runtime-coreclr libraries-jitstress, runtime-coreclr libraries-jitstress2-jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@kunalspathak
Copy link
Member

One of the failing issue is #83316, but there are some arm64 failures in https://dev.azure.com/dnceng-public/public/_build/results?buildId=298474&view=ms.vss-test-web.build-test-results-tab&runId=6039742&resultId=199446&paneView=debug that you might want to check.

@SwapnilGaikwad
Copy link
Contributor Author

One of the failing issue is #83316, but there are some arm64 failures in https://dev.azure.com/dnceng-public/public/_build/results?buildId=298474&view=ms.vss-test-web.build-test-results-tab&runId=6039742&resultId=199446&paneView=debug that you might want to check.

Hi @kunalspathak, I couldn't run the windows on Arm64 failure as I don't have access to such machine. Instead, I tried to reproduce the failure on ubuntu 18.04 as it looked like the similar System.NullReferenceException issue.
I could download the artifacts from the azure pipeline and run them to produce a failure. However, I found the same error when replace the System.Private.CoreLib.dll from the main branch. I'm not sure whether the issue is related to the patch. Do you have any suggestions to proceed on this?

While running the failures from the pipeline, we get a script to reproduce failure that basically runs the following command after setting the required environment variables.

/home/user/dotnet/main/artifacts/bin/testhost/net8.0-linux-Release-arm64/dotnet exec --runtimeconfig \
System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json xunit.console.dll \
System.Private.Xml.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI \
-notrait category=OuterLoop -notrait category=failing 

This runs all the System.Private.Xml.Tests tests. Do you know if we can force it to run a single test? I tried the flags such as -p:XunitMethodName="System.Private.Xml.Tests.XmlSerializerTests" but they don't seem to scope it down.

@kunalspathak
Copy link
Member

I just looked around and noticed that similar tests failed few weeks back and there is #86563 for that. So I think we should be good here. Since they repro with random seeds, the failures is not consistent week over week.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@SwapnilGaikwad
Copy link
Contributor Author

Hi @kunalspathak @EgorBo, is there anything that I can do to push this patch forward? 🙂

@kunalspathak
Copy link
Member

Hi @kunalspathak @EgorBo, is there anything that I can do to push this patch forward? 🙂

Not really. I was waiting for CI to complete. Failures are #87477, #86563, #83316, #70307

@kunalspathak kunalspathak merged commit bec3b04 into dotnet:main Jun 13, 2023
@SwapnilGaikwad SwapnilGaikwad deleted the github-merge-shuffle-guid branch June 29, 2023 12:39
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants