Optimise DisplayClass Allocations by YairHalberstadt · Pull Request #32092 · dotnet/roslyn

YairHalberstadt · 2019-01-02T08:57:01Z

Merge display classes created for closures when it is safe to do so in order to reduce allocations.

…n order to reduce allocations.

…arent

YairHalberstadt · 2019-01-02T09:25:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs

 @"{
-  // Code size       96 (0x60)
-  .maxstack  3
+  // Code size       85 (0x55)


There's a comment at the top of this test:

// IMPORTANT: this code should not initialize any fields in Program.c1.<>c__DisplayClass0 except "a" // Program.c1.<>c__DisplayClass0 should not capture any frame pointers.

I am not fully sure what the comment means, but it will probably have to be changed, as Program.c1.<>c__DisplayClass0 now initialises more fields than just "a"

…e struct DisplayClasses

jaredpar · 2019-01-02T15:58:40Z

CC @agocke

YairHalberstadt · 2019-01-06T12:30:44Z

Here is a quick benchmark showing that at least in some scenarios this can have some quite significant performance improvements:

Method	Mean	Error	StdDev	Median	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
NotMerged	18.67 ns	0.3962 ns	0.9866 ns	18.30 ns	0.0286	-	-	120 B
Merged	12.61 ns	0.2819 ns	0.6589 ns	12.29 ns	0.021	-	-	88 B

Here is the test program:

using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

namespace Benchmarks
{
	[MemoryDiagnoser]
	public class DisplayClassAllocationsBenchmark
	{
		private int _a;
		private int _b;

		public DisplayClassAllocationsBenchmark()
		{
			var rand = new Random();
			_a = rand.Next();
			_b = rand.Next();
		}

		[Benchmark]
		public int NotMerged()
		{
			var a = _a;
			{
				var b = _b;
				Func<int> act = () => a + b;
				return act();
			}
		}

		[Benchmark]
		public int Merged()
		{
			var a = _a;
			var b = _b;
			Func<int> act = () => a + b;
			return act();
		}
	}

	public class Program
	{
		public static void Main(string[] args)
		{
			BenchmarkRunner.Run<DisplayClassAllocationsBenchmark>();
		}
	}
}

YairHalberstadt · 2019-01-06T12:43:56Z

When the function isn't actually run, we get even better performance improvements:

Method	Mean	Error	StdDev	Median	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
NotMerged	9.978 ns	0.2296 ns	0.5457 ns	9.944 ns	0.0133	-	-	56 B
Merged	3.999 ns	0.1080 ns	0.2919 ns	3.885 ns	0.0057	-	-	24 B

using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

namespace Benchmarks
{
	[MemoryDiagnoser]
	public class DisplayClassAllocationsBenchmark
	{
		private int _a;
		private int _b;

		public DisplayClassAllocationsBenchmark()
		{
			var rand = new Random();
			_a = rand.Next();
			_b = rand.Next();
		}

		[Benchmark]
		public int NotMerged()
		{
			var a = _a;
			{
				var b = _b;
                                // we can assume this wont happen - one in 4 billion chance
				if (a + b == 0)
				{
					Func<int> act = () => a + b;
					return act();
				}
				return a + b;
				
			}
		}

		[Benchmark]
		public int Merged()
		{
			var a = _a;
			var b = _b;
                        // we can assume this wont happen - one in 4 billion chance
			if (a + b == 0)
			{
				Func<int> act = () => a + b;
				return act();
			}
			return a + b;
		}
	}

	public class Program
	{
		public static void Main(string[] args)
		{
			BenchmarkRunner.Run<DisplayClassAllocationsBenchmark>();
		}
	}
}

YairHalberstadt · 2019-01-06T12:59:29Z

And in a case where the second display class never ends up needing to be created, and hence merging the display classes cannot improve performance, any loss in performance is insignificant:

Method	Mean	Error	StdDev	Median	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
NotMerged	3.735 ns	0.1222 ns	0.3583 ns	3.559 ns	0.0057	-	-	24 B
Merged	3.766 ns	0.1053 ns	0.2029 ns	3.676 ns	0.0057	-	-	24 B

using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

namespace Benchmarks
{
	[MemoryDiagnoser]
	public class DisplayClassAllocationsBenchmark
	{
		private int _a;
		private int _b;

		public DisplayClassAllocationsBenchmark()
		{
			var rand = new Random();
			_a = rand.Next();
			_b = rand.Next();
		}

		[Benchmark]
		public int NotMerged()
		{
			var a = _a;
			if (a == 0)
			{
				var b = _b;
				Func<int> act = () => a + b;
				return act();
			}
			return a;
		}

		[Benchmark]
		public int Merged()
		{
			var a = _a;
			int b;
			if (a == 0)
			{
				b = _b;
				Func<int> act = () => a + b;
				return act();
			}
			return a;
		}
	}

	public class Program
	{
		public static void Main(string[] args)
		{
			BenchmarkRunner.Run<DisplayClassAllocationsBenchmark>();
		}
	}
}

YairHalberstadt · 2019-01-07T18:42:27Z

Hi @agocke
I was wondering when you might be able to take a look at this, and tell me what's needed?
Thanks

agocke · 2019-01-07T19:13:36Z

Sure, I'll try to get to this today or tomorrow.

YairHalberstadt · 2019-01-07T19:17:16Z

Thanks

agocke · 2019-01-07T23:18:15Z

@YairHalberstadt First thought: did you try doing this by modifying the ScopeBuilder walker to move certain variables into the parent scope? Instead of doing a second pass to unify scopes?

YairHalberstadt · 2019-01-08T04:35:03Z

@agocke
Yes I did. But at that stage you don't yet know whether any other closures capture the parent scope, so this is impossible to do without effecting GC.

agocke · 2019-01-08T18:17:47Z

Gotcha, I think I know where you're going. What would you expect your code to do for the following:

void M()
{
    int a = 0;

    {
         int b = 1;
         Func<int> f = () => a + b;
    }
}

YairHalberstadt · 2019-01-09T06:28:42Z

The same as for

void M()
{
    int a = 0;
    int b = 1;
   {
        Func<int> f = () => a + b;
    }
}

YairHalberstadt · 2019-01-09T06:30:07Z

i.e create a single display class containing both a and b.

YairHalberstadt · 2019-01-09T06:32:25Z

The discussion on the issue is useful, as I explain my approach there. The exact details have changed since then, but the gist is the same.

YairHalberstadt · 2019-01-09T06:34:26Z

The doc comments reflect the final approach

agocke · 2019-01-10T00:53:22Z

Yup, I'll read the discussion.

However, I always think it's good practice to right descriptive commit comments, so I like to see larger changes have

A description of what the current behavior is
A description of what we'd like the new behavior to be
How we're going to do it

agocke · 2019-01-10T02:23:36Z

I think I understand the optimization and I think it's probably worth it. Now I'm just trying to prove soundness.

I seem to remember a similar optimization being discussed in Typed Closure Conversion by Minamide et. al. Did you take a look at that when designing this pass?

YairHalberstadt · 2019-01-10T05:49:41Z

Thanks for the feedback.
I'll keep that in mind for the future.

I haven't seen that discussion. I'll take a look:

For future readers, here is the paper

https://www.google.com/url?sa=t&source=web&rct=j&url=https://www.cs.cmu.edu/~rwh/papers/closures/tr.pdf&ved=2ahUKEwilktKzwuLfAhW1WxUIHXBiB1IQFjAAegQIARAB&usg=AOvVaw1R9FDIliaRxCNgQb8sst0q&cshid=1547099702257

YairHalberstadt · 2019-01-10T06:37:34Z

That paper mentions the idea briefly, and links to this paper:

https://www.google.com/url?sa=t&source=web&rct=j&url=https://pdfs.semanticscholar.org/3383/ac7f0920862241eda3bedaebbce894cc96b2.pdf&ved=2ahUKEwj5_eiix-LfAhXkRxUIHfZEACAQFjABegQIBBAB&usg=AOvVaw0TgLAsGP2mjykE3DvaIcWq&cshid=1547100790806

YairHalberstadt · 2019-01-10T07:09:41Z

However I believe the literature mostly focuses on functional programming languages, which have very different control structures to imperative languages, so the definition of safe optimisations cannot directly be carried over.

I try to preserve both semantics and GC behaviour in this PR.

Semantics are preserved as follows. So long as control flow is linear, it is always semantically preserving to move variable decoration to the outer scope, and therefore it is safe to merge all environments into one.

GOTOs are tricky to analyze, so for now in the presence of one, I simply skip all analysis, and don't merge.

Environments are never merged from inside a loop to outside a loop.

As for GC, if exactly the same set of closures references two different environments, then merging them into one environment cannot increase the lifetime of any of the variables captured by the environment, as they still have the same set of closures referencing them.

If both the above conditions hold, and both environments are classes, we merge the environments.

agocke · 2019-01-10T22:41:13Z

I think the statement is broadly correct, but I don't think your code does that. Specifically, I think all loops are essentially forms of backwards control flow that your code doesn't handle. For instance:

using System;
class C
{
    public static void Main()
    {
        int x = 0;
        for (int y = 0; y < 5; y++)
        {
            for (int i = 0; i < 10; i++)
            {
                Func<int> f = () => i + x;
            }
        }
    }
}

My read is that i and y will both be folded into the same closure environment, but because of the outer for loop, it's not safe to do so. Do you agree?

YairHalberstadt · 2019-01-10T23:16:53Z

I believe my code will not do that, but I will add a test to make sure.

agocke · 2019-03-11T21:22:31Z

@YairHalberstadt, thanks for the reminder! The only thing I have left to do is read through the tests. I'll block out some time this week to go through it.

The one item which I didn't see the last time that I need to look for is coverage of local functions. I'm mainly interested in interleaving scenarios, where captured variables may be in scopes with local functions in between or maybe backwards branches now intervening.

YairHalberstadt · 2019-03-11T21:34:10Z

Thanks.

I'm not quite sure what your asking me to test? Do you mind giving a small example, and an explanation of which particular points you're interested in specifically? Thanks

YairHalberstadt · 2019-03-11T21:41:21Z

Something like this:

using System;
public class C 
{
	public Action M() 
	{
		int a = 1; 
		{
			int b = 2;

			void M() 
			{
				Console.WriteLine(a + b);
			}

			{
				int c = 6;

				void M1() 
				{
					M();
					Console.WriteLine(c);
				}
				return M1;
			}
		}
	}
}

?

agocke · 2019-03-11T22:13:05Z

Yup, that's the idea.

I seem to remember there were also some tricky cases like

int x = 0;
{
    int y = 0;
    void M() => y++;

    {
          Action a = () => x++;
    }
}

Where there was a local function "in between" the capturing of a variable by a lambda, so a closure scope would get created for the parent and then we would have to find the appropriate home for the local function separately (since they don't capture the same variable). It may be that this doesn't present any problems for your analysis, but it's in my standard ideas of breaking closure code now, since it caught so many problems before.

agocke · 2019-03-11T22:14:01Z

Not asking you to add any specific tests, I just need to look at yours and see if I can think of anything that would break :)

YairHalberstadt · 2019-03-12T08:37:09Z

I've added a few more tests, albeit not exactly with a connoisseur's eye. If you can think of anything specific, please let me know.

agocke

This is looking pretty good to me. You may want to include some tests for subtle branches like yield and await. I can't think of a way that those alone could be backwards branches, so I don't think they'll be a problem, but it's a good idea to include a baseline for them anyway.

YairHalberstadt · 2019-03-16T20:48:22Z

I have added a test for yield return, and a test for async await.

agocke

LGTM

agocke · 2019-03-16T21:32:12Z

@VSadov @gafter would you mind reviewing this as well?

YairHalberstadt · 2019-03-26T16:36:37Z

Is it possible to give this a bit of a nudge? :-)
Thanks

VSadov · 2019-03-26T23:04:46Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

                    } while (addedItem == true);

+
                    // Next create the environment and add it to the declaration scope


nit: new empty line

VSadov · 2019-03-26T23:14:17Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

+            /// after the begining of a <see cref="Scope"/>, to a <see cref="BoundLabelStatement"/>
+            /// before the start of the scope, but after the start of <see cref="Scope.Parent"/>.
+            /// 
+            /// All control structures have been converted to gotos and labels by this stage,


All loops have been converted to . . .
Technically there are still switches, try/throw in the tree. However, since you are looking for potential re-execution of lambda expressions without leaving their parent scopes, you only care about backward branches. And loops, which are conveniently lowered to backward branches when we reach here.

VSadov · 2019-03-27T00:09:32Z

I wonder if there are any unwanted effects on debugging. I am not entirely sure.

    class Program
    {
        static string x = "42";

        static void Main(string[] args)
        {
            int y = 123;  // <-- on a breakpoint here, do we see "int x" or "string x" when adding x to Watch window?

            if (true)
            {
                int x = 123;

                Func<int> f = () => x + y;
                System.Console.WriteLine(f());
            }
        }
    }

If there are effects on debugging, perhaps constrain this to -o+ only ?
That would make EnC not a concern as well.

Otherwise looks good.

…isplayClassAllocations

YairHalberstadt · 2019-03-27T09:17:54Z

It did cause the problem in debugging. Fortunately the changes are very localized so switching them off in debug is trivial.

YairHalberstadt · 2019-04-03T15:53:40Z

@VSadov
When do you think you'll have a chance to have another look at this?
Thanks

YairHalberstadt · 2019-04-15T19:33:23Z

@VSadov, @agocke
When do you think it will be possible to finish off this review please?
Thanks

VSadov

LGTM
Thanks!!

agocke · 2019-04-23T17:16:21Z

@YairHalberstadt I'm preparing to merge this shortly, just have to make sure it's going into the right branch and is ready to flow through.

* Revoke IVTs to dotnet/roslyn-analyzers Closes #35102 * Implement InternalsVisibleTo checks Closes #35064 * Allow a work item for tracking migration of IVTs * Exclude MonoDevelop IVTs for not loading inside VS * Exclude Moq IVTs for not loading inside VS * Track removal of external IVTs for XAML See #35069 * Remove IVTs to non-existent assemblies Roslyn.Compilers.CompilerServer.UnitTests removed in 2472120 Roslyn.Test.Utilities.CoreClr removed in cf58bbc Roslyn.Test.Utilities.Desktop removed in cf58bbc Roslyn.Compilers.CSharp.PerformanceTests removed in cd33333 Roslyn.Test.Utilities.FX45 removed in b77c547 Roslyn.Compilers.VisualBasic.PerformanceTests never existed (typo from 3c14b04) Roslyn.Compilers.CSharp.Test.Utilities.Desktop removed in edd89e4 * Track removal of EnC.UnitTests IVTs See #35071 * Track removal of legacy testing IVTs See #35072 * Use the assembly name instead of project file name for IVT analysis * Track removal of project system IVTs See #35070 * Track removal of Live Share IVTs See #35074 * Track removal of F# IVTs See #35076 * Track removal of TypeScript IVTs See #35077 * Track removal of unit testing IVTs See #35078 * Track removal of Razor IVTs See #35079 * Track removal of legacy code analysis IVTs See #35080 * Track removal of internal testing IVTs See #35081 * Remove IVTs to non-existent assemblies Roslyn.InteractiveWindow.UnitTests renamed in 2efc2ce and removed in db7a842 Roslyn.VisualStudio.VisualBasic.Repl renamed in 8ea0f52 and removed in 7ab3b77 Microsoft.VisualStudio.CSharp.Repl removed in 7ab3b77 Microsoft.VisualStudio.VisualBasic.Repl removed in 7ab3b77 * Track removal of IntelliTrace IVTs See #35084 * Track removal of ExternalDependencyServices IVTs See #35085 * Simplify null checks using 'is null' and 'is object' (#35017) * Track removal of CodeLens IVTs See #35086 * Track removal of Scripting.Desktop IVT See #35090 * Track removal of Editor.UI.Wpf IVTs See #35091 * Track removal of Microsoft.VisualStudio.InteractiveServices See #35098 * Track removal of Xamarin IVTs See #35099 * Remove IVTs to non-existent assemblies Roslyn.Compilers.VisualBasic.Test.Utilities.Desktop incorrectly added in a70cdce VBCSCompilerPortable removed in b2bd77b Microsoft.CodeAnalysis.CompilerServer removed in b00224e Roslyn.VisualStudio.Test.Utilities.Next renamed in e0e16d7 Microsoft.CodeAnalysis.Scripting.Destkop.UnitTests never existed (typo) Roslyn.DebuggerVisualizers removed in d7e4939 Microsoft.VisualStudio.LanguageServices.VisualBasic.UnitTests never existed (typo in 7ab3b77) Roslyn.Services.Editor.CSharp.UnitTests2 renamed in bb1f97b * Only process IVTs for the primary solution * Clean up Build Boss code style and error messaging * Fix VisitPatternForRewriting. * install servicehub json files in common7/servicehub folders (#34563) * moved files * opt-in to new "serviceOverride": true support and refactor directory structure to share json files with devdiv insertion projects * added swr for servicehub json files. * delete projects not needed * moving to auto generated service.json approach * made json file included in vsix * generate swr file * address PR feedbacks and remove duplications except vsix manifest * use relative path in json file * share duplicated string to multiple csproj * fix swr package name * PR feedbacks * Add missing binary back-compat method This was missed when we were adding another parameter to the one with optional arguments. * Use Machine.Arm64 instead of raw value. (#35097) Changes Use Machine.Arm64 instead of raw value. These cases were probably missed on #27023. * Add unit-tests for fixed issues. (#35094) Closes #33276. Closes #31676. * Update string * Move to base project so it triggers for CPS * Be resilient to cases where a language service is moving to a new assembly Fixes #34987 * Fix sorting of import completion items * Track IVT removal in Microsoft.CodeAnalysis.Scripting See #5661 * Clarify primary solution argument for build boss * Addressing Review comments. - Added condition to handle cases when invocation is null. - Added corresponding unit test. * Use button over vs:button to have high contrast work correctly. Set the HelpText dynamically * Remove unneeded properties from stack panel * Correctly report null reference possibility when GetEnumerator returns a potentially nullable Enumerator type. * Added test for previously fixed #34667 * Addressed PR feedback. * Update nullable attribute in docs (#34763) * Update nullable attribute in docs Update nullable attribute in docs to show NullableFlags * Update nullable-reference-types.md * Fix the pull member up failure (#34581) * Add missing parameters and tests * Address feedback * Removing extra null check. * PR feedback * `Equals` for generic methods should compare nullable annotations for type type arguments. (#35116) Fixes #35083. * Intellisense broken inside of methods that have delegates as arguments (#35067) * Addressed pr feedback, added new test to demonstrate #35151. * Targeted Completion Prototype * Use tag name instead of literal text * Make target-typed completion an experiment * More strictly keep original code * Rename some helpers * Switch to "CorrelationScope" icon * Add initial tests and telementry * Telemetry * Remove redundant check * Unit tests * Whitespace * Handle multitargeting * More descriptive comment * Rename local * Add a couple tests * Update unit test Trait name * Renaming some things from "matching type" to "target typed" * More renaming from matchingType -> targetTyped * Add VB tests * Update string to "Target type matches" * Rename "MatchingType" to "TargetTypeMatch" more places * Rename to TargetTypedFilters * Improved telemetry * More correct telemetry * Improve linked file handling * Fix & improve telemetry * PR feedback * Definitely avoid some allocations * Remove a repeated allocation. * Formatting * Fix mismerge * Fix mismerges * Fix mismerges * Fix mismerge * Avoid caching VS completion items for non-import items * Regex Completion + Async Completion = Failure to trigger on `[` in VB (#34988) * Fix typos * Cleanup missed prototype comments. * Add link to tracking bug * Async-enumerator methods honor the EnumeratorCancellation attribute (#35121) * Revert "Remove the dependence between the order in NullableAnnotation and metadata attribute values (#34909)" This reverts commit e922628. * Fix complete statement's semicolon placement to better handle incomplete code (#35024) * Fix for 34983 * Cleanup, fix For statements * Add more tests * Respond to feedback * Fix spelling * Binary log for Unix bootstrap Generate a binary log file for the bootstrap phase on Unix platforms. * Add 16.1P3 to .yml files * Update PublishData for Dev16.1 Preview 3 * Use correct branch name for 16.1 Preview 3 * Re-enable set -e * Add test to assert relative order of completion items * Update PublishData for 16.1 Preview 3 and 16.2 Preview 1 * Update eng/config/PublishData.json Co-Authored-By: dpoeschl <dpoeschl@gmail.com> * Update eng/config/PublishData.json Co-Authored-By: dpoeschl <dpoeschl@gmail.com> * Implement IMethodSymbol.ReceiverNullableAnnotation. * Fix to modify tooltip text instead of helptext * Revert "Add tests for experiment service" This reverts commit 42fe72d. * Add import placement codestyle, diagnostic, and fixer (#35009) * Add import placement codestyle option * Add TextEditor CodeStyle options for Using Placement * Add misplaced using directives analyzer and code fix * Use consistent pluralization * Removed Preserve AddImportPlacement option * Removed Preserve from CSharp Style Options * Removed Preserve from editorconfig tests * Coverted to Roslyn style analyzer and fixer tests. * Simplified MisplacedUsings CodeFix based on feedback. * Simplified MisplacedUsings CodeFix based on feedback. * Add warning annotation to moved using directives * Move misplaced usings out of multiple namespaces * Deduplicate usings when moving them * Simplified move usings warning text * Add expected warning markers to misplaced using tests * Fix editor config generator tests * Consolidated diagnostics and tests * Add tests where directives are in both contexts * Update Versions.props for 16.2 * Update Language Feature Status.md * Add version check to enable the pattern-based Index & Range indexers (#35170) * Implement an alternative way to break cycles while calculating IsValueType/IsReferenceType for a type parameter. (#35145) Fixes #30081. * Using FQN instead of adding import during ENC session * Use 'is null' when in C#7 when adding null checks for a parameter. * Don't add null checks for nullable-structs. The fact that they're nullable indicates they don't need checks. * Fix callers * Simplify * Fix * Make bootstrap 32 bit in 32 bit CI This changes our bootstrap compiler to run as a 32 bit process in the 32 bit CI runs. The intent of this runs is to validate we can function on 32 bit systems and that should extend to the compiler as well. This can help us identify the rare cases where we emit code that doesn't perform well on the 32 bit JIT, or just outright crashes. * Don't suggest static members in PropertySubPatternCompletionProvider * Revert two step initialization of base type in PENamedTypeSymbol. (#35189) Related to #28834. Also, remove obsolete comments from a test. Closes #30003. * Ensure we refresh ruleset severities after a ruleset change If a ruleset file changed, we didn't always reapply the new values in the IDE. For legacy projects the IDE is taking the ruleset files and applying them to the compilation; for CPS projects that's being done when we call into the compiler to parse the command line string. We forgot to reparse the command line string, so CPS projects wouldn't refresh until some other command line string changed or the project was unloaded or reloaded. Fixes #35164 * Revert "Revert "Add tests for experiment service"" This reverts commit adbcf8a. * Only emit readonly attributes implicitly when feature enabled (#35213) * Only emit readonly attributes implicitly when readonly members feature is enabled * Comment about reasoning and use Theory for test * Use Properties indexer instead of Add() to track pasted span * VisualBasic semantic model does not recognize overloads at chained queries (#35155) * Add spec for enhanced using (#34697) * Add spec for enhanced using * Create a shared experiment service mock * Fix existing tests * Treat CandidateReason MemberGroup the same as Abiguous when classifying NameSyntax * Rename mock service * Don't provide sync namespace refactoring in generated code * Update azure-pipelines* for dev16.1-preview3* * Update versions.props for 16.1 preview 4 * Update PublishData for 16.1 preview3 and preview4 * Optimise DisplayClass Allocations (#32092) The current implementation of closure conversion creates closure environments for each new scope. This change tries to minimize the number of closure environments, and thus closure environment allocations, when possible by merging "adjacent" closure environments. To merge two closure environments, they have to: 1. Be captured by exactly the same set of closures 2. Have no backwards branching between the allocation of the two environments 3. Have the same environment lifetime If so, all of the variables will be merged into a single closure environment. Fixes #29965 * intellisense should suggest event after readonly in a struct member declaration (#35234) * intellisense should suggest event after writing readonly in a struct member declaration. * Avoid repetition of keywords in EventKeywordRecommender * Re-enable symbol tests on mono (#35265) * Remove usage of QuietRestore (#35264) * Readonly struct and readonly member metadata as source (#34778) * Implement MetadataAsSource for ref and readonly structs * MetadataAsSource for readonly members * Fix NotImplementedException errors. Fix some ArrayBuilder leaks. * Fix a leak. Fix implicit readonly attribute test. * List passed to GetUpdatedDeclarationAccessibilityModifiers needs to support Remove * Allow specifying a metadata language version for MetadataAsSourceTests * Remove unused SyntaxTokenList members. Comment readonly event generation. * Check VB MetadataAsSource for readonly members * Fixes from feedback * Add test for not implicitly readonly struct getter * Label arguments in call to GetCustomAttributesForToken * Reference issue about readonly event public API * Fix crash in pattern matching (#35249) When we relaxed the requirement for pattern matching open types to a constant pattern to not require a conversion from the pattern expression to the open type, but the pattern expression should be required to have a constant value. Fixes #34980 * Warn for CancellationToken parameters missing [EnumeratorCancellation] (#35254) * Use of unannotated unconstrained type parameter in nullable diabled code (#34889) A reference to an unconstrained type parameter in nullable-disabled code should be treated as *oblivious*, and therefore reading them should be considered to produce non-null values, and we are permitted to assign null values to them without a diagnostic. Fixes #34842 Also disable the old WRN_DotOnDefault when the nullable feature is enabled. Fixes #34855 * Error for `typeof(T?)` when `T` is a reference type (#35001) Fixes #29894 * Make Generated syntax trees restore to project-level nullability (#35018) * Make Generated syntax trees restore to project nullability * Focus first tabbable element in PMU dialog (#35212) * Add new optprof test for training * Update foreach based on nullable analysis This makes 2 changes: 1. Reinfer the GetEnumerator method based on nullable analysis of the foreach expression type. 2. Use that information to update the collection element type based on that same analysis. * Update DynamicFileInfo.cs * Fix applying code actions that change AdditionalDocuments Some code was accidentally calling GetDocument with an additional document ID; this didn't end well. * VS 2017 -> VS 2019 * Fix broken link in Language Feature Status Fix link to Nullable reference types specification * Fix more links in Language Feature Status Fix links to recursive patterns and async streams specifications * Move to non Int pools Responding to a request from our core engineering team to move to a different pool. * Remove unnecessary parameter The `CoreCompile` targets for C# and VB were both passing the set of `PotentialEditorConfigFiles` to the `PotentialAnalyzerConfigFiles` input parameter of `CscTask`/`VbcTask`. However, this parameter no longer exists. At one point in the development of the editorconfig-in-compiler feature we had a separate MSBuild task that would compute both the actual and potential .editorconfig file paths and pass them to the task. These are now computed as part of the MSBuild evaluation pass, and the potential .editorconfig files are passed to the project systems via a separate target (`GetPotentialEditorConfigFiles` in Microsoft.Common.CurrentVersion.targets). * Make sure nullability mismatch in constraints specified in different partial declarations (types/methods) are properly detected and reported. (#35272) Make sure nullability mismatch in constraints specified in different partial declarations (types/methods) are properly detected and reported. Fixes #30229. Fixes #35179. Implements the following LDM decision: For partial types, the invariant matching from type inference and merging. A mismatch between two non-oblivious candidates produces an error. No warnings are produced. For partial methods, nullability has to match with exception for oblivious and we produce warnings. For the result, we use the implementation signature inside the implementation, and the declaration signature for the callers. * Make compilation outputs available via a workspace service (#34809) * Handle dynamic null checks against literal null (#34996) Fixes #30939 * Clean up an assertion in LambdaRewriter. (#35284) Fixes #30069 * Fixup from bad merge. (#35351) * Addressed pr feedback. * Lambdas in array initializers checked in nullable walker (#35030) Also fixes a corresponding issue in the switch expression Fixes #34299 See also #35029 * Null inferences do not flow out of a finally block. (#35276) Fixes #34018 * changed the way we report live analysis to task center (#35336) * changed the way we report live analysis to task center previously, we listen to diagnostic service to report progress. problem is that, it only raise event if it found errors on a file. so what we report is actually last file we found errors on rather than file that we are analyzing. this caused confusion since we report in task center that we are analyzing file "A" when it is actually "analyzed" not "analyzing" another issue is since it only report file that contains errors. we might not actually show anything in task center if there is no error, or show file "A" for long time if that is only file with errors. this PR changes the experience closer to what users would expects. and now progress is for solution crawler not specifically on diagnostics. now we report file that solution crawler is analyzing. there is still cavet such as solution cralwer can pause between processing a file if VS is busy. but it will still show file "A". or we will not update UI at least 200ms a part and etc. since it is task center where we don't want to be too impactful to VS, based on feeedback we will see whether we need to do more such as detect solution crawlwer pause and update task center to show pasue. or update task center to show different stage such as analyzing/analyzed. or show in task center, what analyzer is actually running such as diagnostic, todo, designer attribute scan, find all reference cache and etc. * addressing PR feedbacks * Handle val escape for the switch expression. (#35311) Fixes #35278 * Additional Nullability checks for deconstruction: (#35016) * Additional Nullability checks for deconstruction: - Check 'this' param for extension deconstruct - Re-infer the argument types for generic extension deconstruct - Update the deconstruction method in instance cases - Update return type with visited arguments - Update tests * Correct nullability analysis in conditional access (#34973) Fixes #29956 Also introduce a helper `TypeSymbol.IsVoidType()` * Use Button instead of vs:Button on warning dialog for PMU (#35344) * Update version for Microsoft.Net.Test.Sdk package This change is required in order to enable running on non-desktop TFM tests from VS test explorer window. * [master] Update dependencies from dotnet/arcade (#34831) * Update dependencies from https://github.com/dotnet/arcade build 20190407.1 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19207.1 * Update dependencies from https://github.com/dotnet/arcade build 20190409.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19209.2 * Update dependencies from https://github.com/dotnet/arcade build 20190410.7 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19210.7 * Update dependencies from https://github.com/dotnet/arcade build 20190411.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19211.2 * Update dependencies from https://github.com/dotnet/arcade build 20190412.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19212.2 * Update dependencies from https://github.com/dotnet/arcade build 20190413.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19213.2 * Update dependencies from https://github.com/dotnet/arcade build 20190414.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19214.2 * Update dependencies from https://github.com/dotnet/arcade build 20190415.12 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19215.12 * Update dependencies from https://github.com/dotnet/arcade build 20190417.1 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19217.1 * Update dependencies from https://github.com/dotnet/arcade build 20190418.1 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19218.1 * Update dependencies from https://github.com/dotnet/arcade build 20190418.4 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19218.4 * Update dependencies from https://github.com/dotnet/arcade build 20190418.7 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19218.7 * Update dependencies from https://github.com/dotnet/arcade build 20190422.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19222.2 * Update dependencies from https://github.com/dotnet/arcade build 20190423.2 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19223.2 * Use Arcade CoreXT package support * Update dependencies from https://github.com/dotnet/arcade build 20190424.9 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19224.9 * Fix signing * Update dependencies from https://github.com/dotnet/arcade build 20190425.5 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19225.5 * Update dependencies from https://github.com/dotnet/arcade build 20190426.3 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19226.3 * Update dependencies from https://github.com/dotnet/arcade build 20190429.8 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19229.8 * Update dependencies from https://github.com/dotnet/arcade build 20190430.6 - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19230.6 * Use more robust initialization for TypeWithAnnotations.Builder (#35373) * Auto-generate assembly version of the build task assembly (#35238)

Merge display classes created for closures when it is safe to do so i…

31f8c35

…n order to reduce allocations.

YairHalberstadt requested a review from a team as a code owner January 2, 2019 08:57

YairHalberstadt mentioned this pull request Jan 2, 2019

Closure may be unnecessarily split #29965

Closed

Fix XML documentation for LambdaRewriter.ClosureEnvironment.CapturesP…

1c5eada

…arent

YairHalberstadt commented Jan 2, 2019

View reviewed changes

Do not capture parent in LambdaRewriter.Analysis when only parents ar…

ef16385

…e struct DisplayClasses

jaredpar assigned agocke Jan 2, 2019

jaredpar added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 2, 2019

Add extra tests for local methods

e0ebb16

agocke reviewed Mar 16, 2019

View reviewed changes

Add tests for yield return and async await

d218afa

agocke approved these changes Mar 16, 2019

View reviewed changes

VSadov reviewed Mar 26, 2019

View reviewed changes

YairHalberstadt and others added 4 commits March 27, 2019 08:11

Comments and formatting fixed based on code review

35e09e0

Merge remote-tracking branch 'upstream/master' into feature/OptimiseD…

eb43273

…isplayClassAllocations

Only optimise display classes in Release build, not debug

c61faad

Remove whitespace change

dd3fcd6

VSadov approved these changes Apr 18, 2019

View reviewed changes

agocke merged commit d796d86 into dotnet:master Apr 25, 2019

luithefirst mentioned this pull request Feb 16, 2021

Action closure captures unnecessary local variable #51088

Closed

RikkiGibson mentioned this pull request Apr 19, 2021

error MSB6006: "csc.exe" exited with code -2146232797 on switch with type pattern matching and async in release mode. #52639

Closed

		} while (addedItem == true);


		// Next create the environment and add it to the declaration scope

Conversation

YairHalberstadt commented Jan 2, 2019

Uh oh!

YairHalberstadt Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jan 2, 2019

Uh oh!

YairHalberstadt commented Jan 6, 2019

Uh oh!

YairHalberstadt commented Jan 6, 2019

Uh oh!

YairHalberstadt commented Jan 6, 2019

Uh oh!

YairHalberstadt commented Jan 7, 2019

Uh oh!

agocke commented Jan 7, 2019

Uh oh!

YairHalberstadt commented Jan 7, 2019

Uh oh!

agocke commented Jan 7, 2019

Uh oh!

YairHalberstadt commented Jan 8, 2019

Uh oh!

agocke commented Jan 8, 2019

Uh oh!

YairHalberstadt commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Jan 9, 2019

Uh oh!

YairHalberstadt commented Jan 9, 2019

Uh oh!

YairHalberstadt commented Jan 9, 2019

Uh oh!

agocke commented Jan 10, 2019

Uh oh!

agocke commented Jan 10, 2019

Uh oh!

YairHalberstadt commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Jan 10, 2019

Uh oh!

YairHalberstadt commented Jan 10, 2019

Uh oh!

agocke commented Jan 10, 2019

Uh oh!

YairHalberstadt commented Jan 10, 2019

Uh oh!

agocke commented Mar 11, 2019

Uh oh!

YairHalberstadt commented Mar 11, 2019

Uh oh!

YairHalberstadt commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented Mar 11, 2019

Uh oh!

agocke commented Mar 11, 2019

Uh oh!

YairHalberstadt commented Mar 12, 2019

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

YairHalberstadt commented Mar 16, 2019

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

agocke commented Mar 16, 2019

Uh oh!

YairHalberstadt commented Mar 26, 2019

Uh oh!

VSadov Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

VSadov Mar 26, 2019

Choose a reason for hiding this comment

YairHalberstadt commented Jan 9, 2019 •

edited

Loading

YairHalberstadt commented Jan 10, 2019 •

edited

Loading

YairHalberstadt commented Mar 11, 2019 •

edited

Loading