-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Regression impacting ASP.NET MVC #25771
Comments
/cc @mkArtakMSFT |
Hmm, nothing changed in pipelines namespace during those commits. |
This commit was made to System.Memory in that timespan dotnet/corefx@6cc11f5. |
I'm having trouble understanding GitHub's UI for commit diffs, but in the comparison between the commits: |
Strange, dotnet/corefx@573f9e5 doesn't contain pipe changes it shows entire pipe source code as new. |
Also, perfview diff is slightly incorrect as it failed to recognize two casings of |
You can get the two traces here: \scratch2\scratch\sebros\investigation-mvcplaintext-3.27 |
I believe that's the initial Preview2 integration from corefx:master to corefx:/release/2.1; prior to that (preview1) pipelines was coming from https://github.com/dotnet/corefxlab/tree/release/2.1 |
Sharing the commands I used as I have been asked
and
|
I did a bit more digging. Ran benchmarks with It also normalized module names so the current diff looks like ( Pipelines assembly that is used in both runs is the same Module lists: 26314-02
26316-04
|
I isolated the regression in |
CC @jkotas |
FYI, I have the traces from @sebastienros and am investigating. |
It looks like there is a 2.5 second CPU regression here that shows up in System.Private.CoreLib: It looks like it stems from a dictionary lookup in ASP.NET routing. There is more CPU time spent exclusively in these methods (the number on the far right is the exclusive CPU time in milliseconds): Interestingly, when I look at these two commits / versions that you called out as the regression / baseline versions, they both consume the same version of CoreCLR: 2.1.0-preview2-26324-02 https://github.com/dotnet/corefx/tree/23165cee77ae4dd7a7645b7927872948bb27a73e Thus, I am not quite sure how this can happen. @sebastienros, can you confirm that these regression / baseline commits are valid? |
Yes, I also found out about the Dictionary regression, and found some commits around that time. It makes totally sense as the Router is only used in MVC and a big client for dictionary lookups. |
Was this regression only on Windows or also on Linux? |
To be honest I don't know anyone that can correlate the commit numbers (that I got from the assemblies themselves) and what code is in it. It's due to how merges are done in corefx, no rebase/squash, and code copies. The git history is a maze ... it makes it very hard to get meaningful code diffs. A simple way would be to decompile the two dlls I am testing, which I will do asap. |
(I'm wondering if it may have been somehow caused by my changes in dotnet/coreclr#17391. Having the decompiled diff would be helpful.) |
Ah, thanks, definitely not dotnet/coreclr#17391 then, as that wasn't merged until after the drop. Maybe dotnet/coreclr#16945; just speculating based on what Brian shared. |
I shared Brian's question, though. Both of the linked versions are depending on the exact same corelib package: |
I think it's very likely it's due to dotnet/coreclr#16945. Trying to repro now... |
Dug into the trace a bit more and also spoke with @stephentoub. Here are some more stacks that help point at the difference. A good chunk of the regression comes from System.Globalization.CompareInfo.GetIgnoreCaseHash, which was introduced in dotnet/coreclr#16945. |
CC @ViktorHofer @danmosemsft |
This was due to the change to OrdinalIgnoreCase to add in necessary functionality. While that change did decrease perf build-over-build, even with the impact of that one change, .NET Core 2.1’s OrdinalIgnoreCase is still 4-5x faster than it was in .NET Core 2.0 (other changes after this also helped). Closing as by design. |
Testing on a fixed aspnet version and varying dotnet runtime, we see a 6% regression on the MVC Plaintext scenario between these versions:
2.1.0-preview2-26324-02
https://github.com/dotnet/corefx/tree/23165cee77ae4dd7a7645b7927872948bb27a73eand
2.1.0-preview2-26316-06
https://github.com/dotnet/corefx/tree/8130620b542457ca4e72eb2bc81c3eae50192a33A diff on perfview traces for these two versions resulted in this:
The two full traces are available if necessary.
The text was updated successfully, but these errors were encountered: