Skip to content

Keep the host solution pinned on the remote workspace side while performing an inline-rename session#62854

Merged
CyrusNajmabadi merged 30 commits intodotnet:mainfrom
CyrusNajmabadi:renameOOP5
Aug 2, 2022
Merged

Keep the host solution pinned on the remote workspace side while performing an inline-rename session#62854
CyrusNajmabadi merged 30 commits intodotnet:mainfrom
CyrusNajmabadi:renameOOP5

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jul 22, 2022

Inline rename needs to first sync over the solution to OOP to find all the locations to update. Then as a user is typing the new name, we need to issue requests to OOP to determine what edits to make and what conflicts each of the new names they're trying may cause.

This is problemaitc with our current architecture because we have the following set of operations:

  1. Inline rename session starts.
  2. It makes call to oop to find rename locations using solution snapshot S1.
  3. User edits symbol name to something new.
  4. Host syncs to oop solution snapshot S2.
  5. Inline rename calls to oop to find edit/conflict information for snapshot S1 using the new symbol name.

Because 4 and 5 S1 can be dropped. This gets worse as steps 3-5 repeat as the user is typing a new name, so it becomes almost certain that solution snapshot S1 will go away on OOP. THat means that each call to 5 much resync S1 (which is cheap) and then recompute things like compilations for all the projects involved in the rename operation (which is expensive).

Augments the above with the following steps:

After step 1: After session starts, inline-rename calls over to OOP and asks it to pin solution S1.
End: WHen the session ends, we let OOP know it can release S1.

By doing this we ensure that S1 never drops during all the repeats of steps 3-5. This keeps it available in OOP and ensures we don't drop expensive compilations.

@ghost ghost added the Area-IDE label Jul 22, 2022
@CyrusNajmabadi CyrusNajmabadi changed the title WIP not for review. Keep the host solution pinned on the remote workspace side while performing an inline-rename session Jul 29, 2022
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review July 29, 2022 22:32
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner July 29, 2022 22:32
var taskCompletionSource = new TaskCompletionSource<bool>();
cancellationToken.Register(() => taskCompletionSource.TrySetCanceled(cancellationToken));

await taskCompletionSource.Task.ConfigureAwait(false);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the sneaky part. the call from the host to OOP gets here and will block until the entire operation is finally canceled. Because we're inside RunServiceAsync that means we're pinning 'solution' in our internal cache attached to 'solutionChecksum'. So all successive calls to that checksum will see that solution.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to hold onto a thread for this, or does this yield until the task completion source triggers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the latter. there's technically no threads here either (which is good). This is the idea behind a TaskCOmpletionSource, it's a way to get a task that you programatically control the completion of (either giving it a result, an exception, or cancellation).

@CyrusNajmabadi CyrusNajmabadi requested a review from ryzngard August 2, 2022 05:49
@CyrusNajmabadi CyrusNajmabadi merged commit 28a9bc8 into dotnet:main Aug 2, 2022
@ghost ghost added this to the Next milestone Aug 2, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the renameOOP5 branch August 2, 2022 22:07
dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 4, 2022
dibarbet added a commit that referenced this pull request Aug 4, 2022
Revert "Merge pull request #62854 from CyrusNajmabadi/renameOOP5"
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants