Update compiler toolset to latest#69544
Closed
CyrusNajmabadi wants to merge 3 commits intodotnet:mainfrom
Closed
Conversation
ghost
added
Area-Infrastructure
untriaged
jasonmalinowski
approved these changes
Aug 16, 2023
jaredpar
approved these changes
Aug 16, 2023
This was referenced Aug 16, 2023
This was referenced Aug 16, 2023
Member
This reverts commit e1031a7.
This was referenced Aug 17, 2023
Member
|
@RikkiGibson I reopened the two issues linked to the PR that was reverted. Could we provide any more details or a repro on the issue that made it necessary to revert? I see you linked to a source file. What was a the symptom (diagnostic, bad codegen, ...)? |
Member
|
We have some screenshots from Teams which illustrate the unexpected difference in codegen for method
|
Member
|
also, duhh, just pushing the revert in source to here doesn't accomplish anything. We need to build with bootstrap compiler to observe any difference here. |
Member
|
So far, I can manually bootstrap-build the Workspaces dll, and reproduce a failure in the unit test in question. Though, it looks like my stack trace is different locally: in CI: I don't yet grasp exactly what's going wrong and don't have a simplified repro. |
Contributor
Author
|
Are you testing both in release mode? |
Member
|
I ran a debug build of Workspaces project with a debug build of compiler at the last commit of #68694 (be5c937)
#68694 Looks like an innocent change by itself but it's probably a good idea to revert until #69194 can also pass bootstrap. |
Member
Yes though I'm not convinced I have properly reproduced the issue locally. |
Member
|
I ended up downloading the Release mode test artifacts from this CI run and the same from a known good CI run, digging into ILSpy and getting the codegen for the MoveNext method where the issue occurs. I'm attaching the relevant artifacts and decompilation. I found the diff was pretty substantial, too much to just screenshot or paste into a comment. The "good" codegen has IL code size 761 and the "bad" has code size 664. The source code for the FixAllContextsAsync method hasn't changed in 5 months, it looks like. Also apologize that in some of these notes I seem to have mixed up which FixAllContextsAsync method is behaving badly. The stack trace from CI is the source of truth here :) |
Member
|
I think the big difference in the bad codegen is the enumerator being captured in a local (instead of field) This only happens for public async Task M(ImmutableArray<string> a) {
foreach (var i in a) {
M(i.Length is 10 or 20 or 30);
await Task.FromResult(true).ConfigureAwait(false);
}
}Weirdly enough, changing the array to |
Member
|
Thanks for the useful observation. Hmm. I'm still not able to get a minimal reproducer. A program like the following doesn't NRE in either Debug or Release mode, using a compiler from Aug 17th. using System;
using System.Threading.Tasks;
using System.Collections.Immutable;
Console.WriteLine("start");
await M(ImmutableArray.Create("a", "b", "c"));
Console.WriteLine("finish");
public partial class Program
{
public static void M1(bool b) { }
public static async Task M(ImmutableArray<string> a)
{
foreach (var i in a)
{
M1(i.Length is 10 or 20 or 30);
await Task.FromResult(true).ConfigureAwait(false);
}
}
}I am thinking we should just go ahead and merge the revert, take the new toolset when available (hopefully verifying that things work when we do that), and keep these notes available to revisit bringing the optimization back in the future. |
Member
I think it's because FromResult is already completed. I can get an NRE using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.