[Perf -196%] System.Collections.ContainsTrue<Int32> (6) #41167
Comments
DrewScoggins
added
arch-arm32
arch-x86
tenet-performance
Dotnet-GitSync-Bot
added
area-System.Collections
untriaged
|
Tagging subscribers to this area: @eiriktsarpalis |
|
From @adamsitnik It looks like a 32-bit only regression (both ARM and x86). System.Collections.ContainsTrue.ICollection(Size: 512)
|
|
@DrewScoggins I think your diff link above is correct, and matches the x axis on the graph -- it was 6pm 5/9 through midnight. But if I zoom on the graph and do the usual selection of points, it gives me a much wider range -- af1db3e...e18250b Do you get the same result from the graph? |
|
That's because the linked report was run against the installer repo, and since we don't take a lot of updates to the runtime in that repo there can often be big gaps in what runtime is being tested. Since this was such a big gap, I used this link, Array, to try and get a tighter bound for the test. |
|
Why is that graph tighter? Also, it show May 9 08:26 3cd02df through 14:41 6b5850f which gives me 4 commits -- 3cd02df...6b5850f your diff is e77572f...d2f7c8f which strraddles 10 commits. Am I doing it wrong? |
|
That graph is tighter because those runs are from the runtime repo and not the installer repo. So we run closer to every commit. The reason your bound is tighter is that you are better at clicking then I am ;) |
|
What is the difference bewteen runtime repo and installer repo runs? If we have the former why do we do the latter, and the results link above is to the latter? Anyway it's a puzzle as nothing in any of these commit ranges looks suspicions. I also see the perf scenario did not change. |
|
Given it's 32 bit only, and all collections are affected, my suspicion is codegen. But there's almost no code in common between these scenarios! From https://pvscmdupload.blob.core.windows.net/reports/08_20_2020/report_Daily_ca=x86_cb=master_co=Windows1018362_cr=dotnetcoresdk_cc=CompliationMode=tiered-RunKind=micro_Baseline_bb=release-3.1.1xx_2020-08-20.html it's affecting Seems unlikely, but if we got our diff a bit wrong, this commit could be implicated? @CarolEidt can you imagine how this would affect this scenario? |
|
@DrewScoggins were there any other scenarios regressed in this range, other than ContainsTrue/ContainsFalse? |
On the performance team we have two goals that we try to meet. One, is that we understand the performance of the product in the way that our customers will use it, and the second is that we provide good data for developers to be able to fix regressions and understand improvements they make. To meet this first goal we run against the installer repo. because this is what we ship to customers. We want to ensure that between what happened in the runtime repo and being ingested into the installer we didn't introduce some kind of regression, or that there is some interaction with other system components that caused this. But, because it can often be many commits between the runtime repo before changes get ingested into installer, we decided we needed finer-grained data. This is why we also do runs on the runtime repo, because they give us the data and tight regression bounds that make investigation simpler. Hopefully, that explains why we run both. Now, as to why the above results link is for the installer repo, that is because at the time of the 3.1 release we only had runs on the installer repo. So, in order to make an apples to apples comparison we used the installer repo runs to generate these regressions. We can use, and have as seen in this issue, the runtime runs as a supplement to help with investigation. However, going forward I believe are plan will still be to use the installer repo for release signoff, as that gives us the highest fidelity to what we plan to ship to customers. |
|
@DrewScoggins thanks for the detailed explanation, that makes sense. Yes, the fine grained data is very important to us, so it makes sense to run against both. |
|
The only other change in taht range that could even vaguely have affected this is @jkotas 's 6b5850f but I see nothing there. Could you help me @DrewScoggins with my other question: were there any other scenarios regressed in this range, other than ContainsTrue/ContainsFalse? |
|
You can find the report here, but a few caveats. Because this is just comparing two back-to-back builds we don't get to run many of the filters that reduce noise on the list of regressions. So that report will contain a decent amount of noise, but looking through I can see that many other tests are showing the same pattern as the ContainsTrue/ContainsFalse tests, and regressed over this bound. |
|
Yeesh.. yes, all regressing at the same point. Here's the worst ones: These are clearly step functions. Do we have issues open for them all? Eg., this one ? |
|
It was caused by my change. I will have a PR up shortly. |
|
I was looking all over that! What was it? I looked pretty carefully and didn't find it yet |
|
@DrewScoggins one thing worries me here - we had a bunch of scenarios do a clear step function regression of up to 250% in early May - why didn't we know about this until Aug 21st? |
|
Never mind, subtraction has precedence over comparison operators, but it was clearer before with the parentheses. |
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before. The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions. Fixes dotnet#41167
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before. The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions. Fixes #41167
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before. The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions. Fixes #41167
adamsitnik
removed
tenet-performance-benchmarks
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before. The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions. Fixes #41167 Co-authored-by: Jan Kotas <jkotas@microsoft.com>
I would still like to understand this @DrewScoggins @adamsitnik .. not interested in blaming anyone 😁 just making sure we understand and can improve our processes if necessary. thoughts? |
The issue was reported by the bot in June: DrewScoggins/performance-2#910 (comment) We meet once a week with Drew, Tanner, Andy an Kunal and go throught the list of all reported issues. We have most probably not discussed this particular one (human error). I've confirmed it during my independent verificaiton: DrewScoggins/performance-2#910 (comment) and @DrewScoggins moved it here. So far this was the only issue like this, all other x86 and x64 issues were already reported (in general the initial results of my verification looks very promissing). My plan is to send a report when I am done and also a PR to the bot repo that adds all the actual regressions as unit tests to ensure that we don't loose the precision over time. @danmosemsft does it sound good to you? |
|
Sounds good! Again, just interested in ways we can continue to help remove opportunities for error from our processes, as we go along. Thanks! |
ghost
locked as resolved and limited conversation to collaborators
Run Information
Regressions in System.Collections.ContainsTrue<Int32>
Historical Data in Reporting System
Repro
Histogram
System.Collections.ContainsTrue.Array(Size: 512)
System.Collections.ContainsTrue.ImmutableArray(Size: 512)
System.Collections.ContainsTrue.List(Size: 512)
System.Collections.ContainsTrue.Queue(Size: 512)
System.Collections.ContainsTrue.ICollection(Size: 512)
System.Collections.ContainsTrue.Span(Size: 512)
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: