Remove OOP related feature options. #31644

CyrusNajmabadi · 2018-12-09T04:32:36Z

This code to make it possible to run features OOP was added around 18 months ago. Since then there has been no issue with it. It has also been A/B tested with no stability issues arising, and positive numbers for all latency-oriented scenarios.

This PR simply removes the options for controlling this and makes it so that OOP is on all the time.

Fixes #26076

CyrusNajmabadi · 2018-12-09T04:33:47Z

src/Workspaces/Core/Portable/Remote/RemoteFeatureOptions.cs

-        public static readonly Option<bool> SymbolFinderEnabled = new Option<bool>(
-            nameof(RemoteFeatureOptions), nameof(SymbolFinderEnabled), defaultValue: true,
-            storageLocations: new LocalUserProfileStorageLocation(LocalRegistryPath + nameof(SymbolFinderEnabled)));
-
        public static readonly Option<bool> DiagnosticsEnabled = new Option<bool>(
            nameof(RemoteFeatureOptions), nameof(DiagnosticsEnabled), defaultValue: true,
            storageLocations: new LocalUserProfileStorageLocation(LocalRegistryPath + nameof(DiagnosticsEnabled)));


this option is still around since some tests explicitly set this to false as diagnostics doesn't work OOP in tests (not sure why, but i'm just preserving that behavior).

@heejaechang Do you know why diagnostics can't run OOP during tests? The relevant check that forces us to keep this is here:

roslyn/src/EditorFeatures/TestUtilities/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs

Lines 375 to 379 in f79608d

using (var workspace = CreateWorkspaceFromOptions(initialMarkup, parameters))

{

// Currently, OOP diagnostics don't work with code action tests.

workspace.Options = workspace.Options.WithChangedOption(

RemoteFeatureOptions.DiagnosticsEnabled, false);

test doesn't have OOP. you need to explicitly set up mock remote host to simulate OOP in unit test which I do for VS.Net unit test but nowhere else.

CyrusNajmabadi · 2018-12-09T04:34:08Z

Tagging @sharwell @jinujoseph @dotnet/roslyn-ide

CyrusNajmabadi · 2018-12-09T04:35:26Z

src/EditorFeatures/CSharpTest/AddUsing/AddUsingTests.cs

-                                                 .WithChangedOption(RemoteFeatureOptions.OutOfProcessAllowed, outOfProcess)
-                                                 .WithChangedOption(RemoteFeatureOptions.AddImportEnabled, outOfProcess);
-
+            workspace.Options = workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, outOfProcess);


note: these features fallback to running proc if the oop server doesn't launch. So it's till good to ahve tests that validate they work properly both in proc and out of proc.

jinujoseph · 2018-12-10T09:48:41Z

@heejaechang ,can we rerun the val build again now that the insertion is done

heejaechang · 2018-12-10T18:51:25Z

src/Workspaces/Core/Portable/Remote/RemoteHostClientExtensions.cs

-
-            return workspace.TryGetRemoteHostClientAsync(cancellationToken);
-        }
+            => workspace.Options.GetOption(featureOption);


not sure why we still have this option.

You're right. Removed. Thanks! :)

heejaechang · 2018-12-10T18:51:45Z

src/Workspaces/Core/Portable/Remote/RemoteFeatureOptions.cs

-
-        public static bool AnyFeatureRunsInProcess(Workspace workspace)
-            => AllFeatureOptions.Any(o => !workspace.IsOutOfProcessEnabled(o));
-
        public static bool ShouldComputeIndex(Workspace workspace)


not sure why we still need this.

This is still necessary because computing the index should only happen in certain workspaces. i.e. we do not want to generate the indices in the VS workspaces. But we do want them in the remote workspace :) Thanks!

heejaechang · 2018-12-10T18:52:37Z

I had similar PR (#29676) and closed because @sharwell said he has PR with different approach.

CyrusNajmabadi · 2018-12-10T19:15:25Z

I had similar PR (#29676) and closed because @sharwell said he has PR with different approach.

Not sure what's up then. AFAICT, OOP is still not enabled by default, and the option for the user is still in the product.

sharwell · 2018-12-10T19:19:24Z

Probable duplicate of #27983 or #27985.

CyrusNajmabadi · 2018-12-10T19:25:39Z

@sharwell there seems to have been no movement there for like 6 months :)

sharwell · 2018-12-10T19:30:14Z

We're going to review the three approaches in design meeting today and move one forward.

CyrusNajmabadi · 2018-12-10T19:31:23Z

We're going to review the three approaches in design meeting today and move one forward.

Ah. Good to know. Feel free to close this out if you prefer another approach. I didn't realize this was something being worked on :)

CyrusNajmabadi · 2018-12-17T21:32:45Z

Can this be merged in @sharwell @jasonmalinowski ?

CyrusNajmabadi · 2019-01-07T21:34:23Z

Can this be merged in @sharwell @jasonmalinowski ?

sharwell · 2019-01-08T00:11:09Z

@CyrusNajmabadi I believe we are planning to merge #27983 first. If this pull request has other changes we can look into it as a follow-up.

CyrusNajmabadi · 2019-02-06T18:57:27Z

@sharwell @jasonmalinowski now that #27983 has gone in, this is all that's left. Plz review and merge when convenient for you. Thanks!

CyrusNajmabadi · 2019-02-18T08:22:58Z

@sharwell @jasonmalinowski now that #27983 has gone in, this is all that's left. Plz review and merge when convenient for you. Thanks!

sharwell · 2019-02-18T18:59:03Z

src/EditorFeatures/CSharp/CompleteStatement/CompleteStatementCommandHandler.cs

+            => forStatementSyntax.Condition == null
+                ? false
+                : caretPosition > forStatementSyntax.Condition.SpanStart &&
+                  caretPosition < forStatementSyntax.Condition.Span.End;


📝 This change can be reverted (this file is out of scope for the current PR)

@sharwell i was getting a warning that the formatting here was incorrect. I like having 0 warnings locally. So i fixed it up. Otherwise i think you can just get warning fatigue.

CyrusNajmabadi · 2019-02-19T19:01:21Z

@sharwell Feel free to merge when ready.

CyrusNajmabadi · 2019-02-22T20:04:41Z

ping @sharwell @jinujoseph can we move forward with this?

sharwell · 2019-02-24T05:36:46Z

@CyrusNajmabadi can you update the title of this PR to reflect the new behavior?

CyrusNajmabadi · 2019-02-24T06:40:40Z

@sharwell done.

CyrusNajmabadi · 2019-02-25T00:18:29Z

@sharwell You thumbsupped... but didn't merge :)

heejaechang · 2019-03-01T19:48:55Z

talked with @sharwell and @jinujoseph and I am going to take this.

CyrusNajmabadi · 2019-03-01T20:05:38Z

Thanks!

* dotnet/master: (903 commits) Add UseEnhancedColors to FeatureOnOffOptionsProvider (dotnet#33802) Update TypeStyle.cs Text on a InterpolatedVerbatimStringStartToken token (dotnet#33747) added ability to clear all diagnostics reproted from a IDiagnosticUpd… (dotnet#33805) Use a set instead of map to define processed redundant nodes. EnC: Remove dependency on solution from AnalyzeDocumentAsync (dotnet#33796) Add workitem links to new redundant assignment tests. Move leading trivia with node when removing unused values. Remove OOP related feature options. (dotnet#31644) Merge pull request dotnet#33631 from CyrusNajmabadi/wrapPriority Use the correct iteration count in IterationDataAttribute Handle interface members in NullableWalker.AsMemberOfType (dotnet#33764) Cleanly work around microsoft/nodejstools#2138 Implement full spec changes for Index/Range (dotnet#33679) Further reduce expectations on deep fluent calls Lower expectation for deep fluent call Take screenshot after writing logs Avoid throwing exception in static constructor Offer to add parameter to constructor with no existing parameters (dotnet#33624) Add regression test for nullable crash (dotnet#33735) ...

CyrusNajmabadi requested a review from a team as a code owner December 9, 2018 04:32

CyrusNajmabadi commented Dec 9, 2018

View reviewed changes

jcouv added the Area-IDE label Dec 9, 2018

jinujoseph assigned heejaechang Dec 10, 2018

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 10, 2018

jasonmalinowski approved these changes Dec 10, 2018

View reviewed changes

jinujoseph added this to the 16.0.P2 milestone Dec 10, 2018

heejaechang reviewed Dec 10, 2018

View reviewed changes

jinujoseph modified the milestones: 16.0.P2, 16.1 Jan 17, 2019

CyrusNajmabadi added 2 commits February 6, 2019 10:55

Enable OOP features by default.

1e0d820

Removing unused function.

bcb6d00

CyrusNajmabadi force-pushed the enableOop branch from 0a735c8 to bcb6d00 Compare February 6, 2019 18:55

CyrusNajmabadi requested review from a team as code owners February 6, 2019 18:55

CyrusNajmabadi changed the base branch from master to dev16.0.x February 6, 2019 18:55

CyrusNajmabadi requested a review from a team as a code owner February 6, 2019 18:55

CyrusNajmabadi changed the base branch from dev16.0.x to dev16.1-preview1 February 6, 2019 18:55

conflicts

5f0a5e9

jasonmalinowski approved these changes Feb 6, 2019

View reviewed changes

RikkiGibson changed the base branch from dev16.1-preview1 to master February 8, 2019 05:43

Merge branch 'master' into enableOop

ca34895

sharwell reviewed Feb 18, 2019

View reviewed changes

sharwell approved these changes Feb 18, 2019

View reviewed changes

CyrusNajmabadi changed the title ~~Enable OOP features by default.~~ Remove OOP related feature options. Feb 24, 2019

heejaechang merged commit 6f5be2e into dotnet:master Mar 1, 2019

jinujoseph modified the milestones: 16.1, 16.1.P1 Mar 1, 2019

CyrusNajmabadi deleted the enableOop branch March 1, 2019 20:05

sharwell mentioned this pull request May 2, 2019

🔼 Compute indices when OOP is disabled #34793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove OOP related feature options. #31644

Remove OOP related feature options. #31644

CyrusNajmabadi commented Dec 9, 2018 •

edited

Loading

CyrusNajmabadi Dec 9, 2018

CyrusNajmabadi Dec 9, 2018 •

edited

Loading

heejaechang Dec 10, 2018

CyrusNajmabadi commented Dec 9, 2018

CyrusNajmabadi Dec 9, 2018

jinujoseph commented Dec 10, 2018

heejaechang Dec 10, 2018

CyrusNajmabadi Dec 10, 2018

heejaechang Dec 10, 2018

CyrusNajmabadi Dec 10, 2018 •

edited

Loading

heejaechang commented Dec 10, 2018

CyrusNajmabadi commented Dec 10, 2018

sharwell commented Dec 10, 2018 •

edited

Loading

CyrusNajmabadi commented Dec 10, 2018

sharwell commented Dec 10, 2018

CyrusNajmabadi commented Dec 10, 2018

CyrusNajmabadi commented Dec 17, 2018

CyrusNajmabadi commented Jan 7, 2019

sharwell commented Jan 8, 2019

CyrusNajmabadi commented Feb 6, 2019

CyrusNajmabadi commented Feb 18, 2019

sharwell Feb 18, 2019

CyrusNajmabadi Feb 18, 2019

CyrusNajmabadi commented Feb 19, 2019

CyrusNajmabadi commented Feb 22, 2019

sharwell commented Feb 24, 2019

CyrusNajmabadi commented Feb 24, 2019

CyrusNajmabadi commented Feb 25, 2019

heejaechang commented Mar 1, 2019

CyrusNajmabadi commented Mar 1, 2019

	using (var workspace = CreateWorkspaceFromOptions(initialMarkup, parameters))
	{
	// Currently, OOP diagnostics don't work with code action tests.
	workspace.Options = workspace.Options.WithChangedOption(
	RemoteFeatureOptions.DiagnosticsEnabled, false);

Remove OOP related feature options. #31644

Remove OOP related feature options. #31644

Conversation

CyrusNajmabadi commented Dec 9, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Dec 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 9, 2018

Choose a reason for hiding this comment

jinujoseph commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang commented Dec 10, 2018

CyrusNajmabadi commented Dec 10, 2018

sharwell commented Dec 10, 2018 • edited Loading

CyrusNajmabadi commented Dec 10, 2018

sharwell commented Dec 10, 2018

CyrusNajmabadi commented Dec 10, 2018

CyrusNajmabadi commented Dec 17, 2018

CyrusNajmabadi commented Jan 7, 2019

sharwell commented Jan 8, 2019

CyrusNajmabadi commented Feb 6, 2019

CyrusNajmabadi commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 19, 2019

CyrusNajmabadi commented Feb 22, 2019

sharwell commented Feb 24, 2019

CyrusNajmabadi commented Feb 24, 2019

CyrusNajmabadi commented Feb 25, 2019

heejaechang commented Mar 1, 2019

CyrusNajmabadi commented Mar 1, 2019

CyrusNajmabadi commented Dec 9, 2018 •

edited

Loading

CyrusNajmabadi Dec 9, 2018 •

edited

Loading

CyrusNajmabadi Dec 10, 2018 •

edited

Loading

sharwell commented Dec 10, 2018 •

edited

Loading