Export option persisters via IOptionPersisterProvider by sharwell · Pull Request #52063 · dotnet/roslyn

sharwell · 2021-03-23T01:20:45Z

Closes AB#761238

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

JoeRobich · 2021-03-23T17:10:49Z

src/VisualStudio/Core/Def/Implementation/Options/LocalUserRegistryOptionPersister.cs

        {
-            // Starting in Dev16, the ILocalRegistry service behind this call is free-threaded, and since the service is offered by msenv.dll can be requested
-            // without any marshalling (explicit or otherwise) to the UI thread.
+            threadingContext.ThrowIfNotOnUIThread();


Why add this assert? Was the earlier comment incorrect?

the comment was not supported by public documentation, so I removed it

Also note that the implementation itself is not intended for use on background threads:
https://devdiv.visualstudio.com/DevDiv/_git/VS?path=%2Fsrc%2Fenv%2Fshell%2FPackageFramework%2FCurrent%2FShell%2FVSRegistry.cs&version=GBmain&line=63&lineEnd=64&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

I think the trick with this one is it's special cased:

https://devdiv.visualstudio.com/DevDiv/_git/VS?path=%2Fsrc%2Fenv%2Fmsenv%2Fcore%2Fproffer.cpp&version=GBmain&line=766&lineEnd=775&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

https://devdiv.visualstudio.com/DevDiv/_git/VS?path=%2Fsrc%2Fenv%2Fmsenv%2Fcore%2Fproffer.cpp&version=GBmain&line=1737&lineEnd=1738&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

This is why #52063 (comment) is the only sane approach to handling this...

As far as I'm concerned, the comment was the public documentation. 😄

I don't mind the assert now being there given the larger context is now doing the call on the UI thread, but can we keep the comment? Or revise it to explain why the assert is there? I expect this to move around further and having to go through this again would be unfortunate.

Documentation is now more complete

the assert feels weird to me. this was made free threaded and communicated to us that way. if we think their documentation should be more sufficient, i woudl prefer we just ask them to do that.

...lStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersisterProvider.cs

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

CyrusNajmabadi

looking for clarity on one bit. otherwise, it looks fine to me. do we need to rope in TS/F#? do they do anything with option persistence?

jasonmalinowski

So I suspect the behavior where the global option service is caching a list, and we drop uncreated ones permanently is going to be broken. If anybody were to fetch the VisualStudioWorkspace prior to our package being loaded (which is totally allowed), then we'll cache an empty list and things will be broken for the rest of that session. A specific thing to test would be opening a CPS based project that doesn't have any saved files open. We won't load our package there (because it's only a perf hit) but the workspace will have been created which will trigger reading of various options at that point. We very much may need to add some code that says if we're running on the UI thread and the persisters aren't created by the package, we still create them synchronously at that point to ensure this doesn't regress something. Otherwise the regression risk here is very high.

More generally it's not clear to me what goodness this is giving here. The bug linked nor PR description doesn't actually make it clear to me which assembly load we're trying to move off the UI thread here -- the MEF components being created at least all exist in the same assembly as RoslynPackage, So my assumption is if this is some other dependency (the workspaces layer? something else?) then I'm not sure how this reliably ensures we don't end up triggering that load somewhere else earlier. If nothing else, I'd appreciate the PR description giving a bit of a background on the internal bug, because the internal bug is pretty opaque.

As far as general direction, I think we're far better off deleting the persister preloading entirely, which I think is achievable but have a few details to still work out. So this work might get deleted soon enough. But since this is written, and it's applying a straightforward refactoring in a consistent way, I can't fault it for making a crappy corner of our codebase a bit less crappy. I'll happily sign off once we know we don't have functional bugs here (see first paragraph.)

(not marking as "request changes" or "approve" just because it went back to draft after my private promise to review it today, and whereas I think it needs changes I think that's probably why it went back to draft.)

...lStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersisterProvider.cs

src/Features/Core/Portable/EditAndContinue/EditSession.cs

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

jasonmalinowski

Looking pretty good just some small requests for docs, a rename to make the workspace threading service clear it's being used for options.

...lStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersisterProvider.cs

src/Workspaces/Core/Portable/Shared/Utilities/IWorkspaceThreadingService.cs

jasonmalinowski · 2021-03-25T01:15:01Z

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

+                return optionSerializers;
+            }
+
+            ImmutableInterlocked.InterlockedInitialize(


Don't we have an EnsureInitialized helper for this pattern somewhere?

jasonmalinowski · 2021-03-25T01:18:35Z

src/VisualStudio/Core/Def/Implementation/Options/LocalUserRegistryOptionPersister.cs

        {
-            // Starting in Dev16, the ILocalRegistry service behind this call is free-threaded, and since the service is offered by msenv.dll can be requested
-            // without any marshalling (explicit or otherwise) to the UI thread.
+            threadingContext.ThrowIfNotOnUIThread();


I don't mind the assert now being there given the larger context is now doing the call on the UI thread, but can we keep the comment? Or revise it to explain why the assert is there? I expect this to move around further and having to go through this again would be unfortunate.

CyrusNajmabadi · 2021-03-29T17:18:42Z

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

            }
        }

+        private ImmutableArray<IOptionPersister> GetOptionPersisters()


should this become async/cancellable in teh future?

Yes, but it is a much more involved change. I wouldn't expect it to happen unless it became required as a result of some independent observation of a problem.

CyrusNajmabadi · 2021-03-29T17:19:47Z

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs

+                else
+                {
+                    return GetOptionPersistersAsync(optionSerializerProviders, cancellationToken).WaitAndGetResult_CanCallOnBackground(cancellationToken);
+                }


instead of getting the service, checking for null, and falling back to WaitAndGetResult here, should we instead export a DefaultWorkspaceThreadingService that has this as its implementation for .Run

This is not a workspace service, so we can only export one total instance of it (MEF layering is specific to our workspace and language services). Exporting a default would make it very difficult to override.

A two-layer indirection alternative exists, which I can implement separately to see if it's preferred.

CyrusNajmabadi

I'm ok with this if jason is :)

jmarolf · 2021-03-29T18:06:52Z

src/EditorFeatures/Core/Shared/Utilities/WorkspaceThreadingService.cs

+namespace Microsoft.CodeAnalysis.Editor.Shared.Utilities
+{
+    [Export(typeof(IWorkspaceThreadingService))]
+    [Shared]


What is the style here? I typically keep the Shared attribute on the same line

I always put one attribute in one set of brackets

jmarolf · 2021-03-29T18:14:43Z

src/VisualStudio/Core/Def/Implementation/TaskList/CommentTaskTokenSerializerProvider.cs

+namespace Microsoft.VisualStudio.LanguageServices.Implementation.TaskList
+{
+    [Export(typeof(IOptionPersisterProvider))]
+    internal sealed class CommentTaskTokenSerializerProvider : IOptionPersisterProvider


All of these IOptionPersisterProvider's look very similar, can we combine most of this into a common base class?

This was covered by #52063 (comment). In this case, sharing doesn't bring enough value to cover its own complexity cost.

sharwell added 2 commits March 22, 2021 18:17

Pass CancellationToken to SelectAsArrayAsync

b90760f

Export option persisters via IOptionPersisterProvider

3cdcb84

ghost added the Area-IDE label Mar 23, 2021

sharwell marked this pull request as ready for review March 23, 2021 02:37

sharwell requested review from a team as code owners March 23, 2021 02:37

sharwell force-pushed the persister-provider branch from 1feae75 to 69ac04b Compare March 23, 2021 04:26

sharwell added 2 commits March 23, 2021 09:53

Avoid loading packages synchronously

b6c096a

Closes dotnet#37377

Avoid loading assemblies on the UI thread

fcc29a2

Closes AB#761238

sharwell force-pushed the persister-provider branch from 69ac04b to fcc29a2 Compare March 23, 2021 16:54

JoeRobich reviewed Mar 23, 2021

View reviewed changes

JoeRobich approved these changes Mar 23, 2021

View reviewed changes

CyrusNajmabadi reviewed Mar 23, 2021

View reviewed changes

...lStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersisterProvider.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 23, 2021

View reviewed changes

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs Show resolved Hide resolved

CyrusNajmabadi suggested changes Mar 23, 2021

View reviewed changes

sharwell marked this pull request as draft March 24, 2021 00:44

jasonmalinowski reviewed Mar 24, 2021

View reviewed changes

...lStudio/Core/Def/Implementation/Options/RoamingVisualStudioProfileOptionPersisterProvider.cs Show resolved Hide resolved

src/Features/Core/Portable/EditAndContinue/EditSession.cs Show resolved Hide resolved

src/Workspaces/Core/Portable/Options/GlobalOptionService.cs Show resolved Hide resolved

sharwell force-pushed the persister-provider branch from 8db3bd8 to 6459317 Compare March 24, 2021 16:39

Ensure GlobalOptionService always has persister instances

b92dc39

sharwell force-pushed the persister-provider branch from 6459317 to b92dc39 Compare March 24, 2021 18:37

sharwell marked this pull request as ready for review March 24, 2021 23:25

jasonmalinowski reviewed Mar 25, 2021

View reviewed changes

sharwell added 2 commits March 24, 2021 20:28

Update documentation

b7a924e

Null-annotate RoamingVisualStudioProfileOptionPersister

32b901b

CyrusNajmabadi reviewed Mar 29, 2021

View reviewed changes

CyrusNajmabadi approved these changes Mar 29, 2021

View reviewed changes

Merge remote-tracking branch 'dotnet/main' into persister-provider

c3013b8

jmarolf reviewed Mar 29, 2021

View reviewed changes

sharwell merged commit 7b9df38 into dotnet:main Mar 29, 2021

ghost added this to the Next milestone Mar 29, 2021

sharwell deleted the persister-provider branch March 29, 2021 22:16

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Conversation

sharwell commented Mar 23, 2021 • edited by Pilchie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sharwell commented Mar 23, 2021 •

edited by Pilchie

Loading

sharwell Mar 29, 2021 •

edited

Loading