Skip to content

Conversation

@tmat
Copy link
Member

@tmat tmat commented Jul 1, 2020

Moves EnC workspace service OOP.

The services that remain in-proc are ActiveStatementTrackingService and EditAndContinueDiagnosticAnalyzer. The former needs to be in-proc due to dependency on the editor tracking spans. Once the editor provides remote syncing capabilities we'll be able to move it OOP. The latter is a simple analyzer proxy that calls to OOP to perform the actual analysis. We'll be able to remove this once we move to diagnostic pull model.

EditAndContinueWorkspaceService is called via RemoteEditAndContinueServiceProxy. The proxy either calls the local service (if Roslyn runs w/o OOP) or serializes calls to RemoteEditAndContinueService, which in turn deserializes the inputs and calls remote instance of EditAndContinueWorkspaceService.

Fixes #42179

Includes #46826

Blocked on RPS regressions:

Further possible improvements:

@tmat
Copy link
Member Author

tmat commented Jul 1, 2020

@sharwell Should should semi-work (other than tracking the active statements).

@tmat tmat force-pushed the RemoteEncService branch 2 times, most recently from beaa1d9 to ae971e6 Compare October 14, 2020 22:20
@tmat tmat changed the title Move EnC to OOP Move EnC OOP Oct 20, 2020
@tmat tmat force-pushed the RemoteEncService branch 3 times, most recently from 2fc9f18 to 46c804a Compare October 26, 2020 19:59
@tmat tmat force-pushed the RemoteEncService branch from 46c804a to 74955db Compare October 30, 2020 18:32
@tmat tmat force-pushed the RemoteEncService branch 2 times, most recently from 8220551 to e884786 Compare November 26, 2020 00:34
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, though its largely moving around bits of code I don't understand :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nit, because naming is hard, but I feel like Proxy is not ideal given the logic here and in other methods, and perhaps could be renamed in a follow up.

It feels like this class is the real EnC service, and using just the service on its own would be surprising because callers would have to know what extra work to do.

Copy link
Member Author

@tmat tmat Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using the real service would be surprising because that service does not have any side-effects. It returns the diagnostics and the caller then is responsible for reporting them. The diagnostic handling here is just a workaround for not having pull diagnostics model and will go away once we remove solution crawler.

That said, I'm open to suggestions of a better name.

@tmat tmat marked this pull request as ready for review December 2, 2020 01:47
@tmat tmat force-pushed the RemoteEncService branch 3 times, most recently from 3d5d72a to 529f518 Compare December 4, 2020 23:07
@tmat tmat changed the title Move EnC OOP Move EnC OOP - part 1 Dec 16, 2020
@tmat tmat force-pushed the RemoteEncService branch from 3f72db5 to 553ab2a Compare January 7, 2021 01:18
@tmat tmat force-pushed the RemoteEncService branch from 553ab2a to 3bbddc7 Compare January 12, 2021 23:17
@tmat tmat force-pushed the RemoteEncService branch from 3bbddc7 to 7413d12 Compare January 12, 2021 23:19
@tmat tmat changed the base branch from master to release/dev16.10 January 12, 2021 23:19
@tmat tmat requested a review from a team January 12, 2021 23:19
@tmat tmat changed the base branch from release/dev16.10 to master January 12, 2021 23:21
@tmat
Copy link
Member Author

tmat commented Jan 12, 2021

Superseded by #50410

@tmat tmat closed this Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Interactive Concept-OOP Related to out-of-proc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move Edit and Continue OOP

4 participants