Skip to content

Use fast in-proc heuristic to implement HasChangesAsync#61690

Merged
tmat merged 9 commits intodotnet:mainfrom
tmat:EncGetChanges
Jun 6, 2022
Merged

Use fast in-proc heuristic to implement HasChangesAsync#61690
tmat merged 9 commits intodotnet:mainfrom
tmat:EncGetChanges

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Jun 3, 2022

EditAndContinueLanguageService.HasChangesAsync is called on UI thread when stepping to determine whether any changes have been made that the debugger needs to apply before resuming the execution of the debuggee. Previously we called to the EnC OOP service to determine the changes precisely. This determination may involve running code generators and checking document content against PDB checksums, which may cause long UI delays.

@isadorasophia pointed out that precision is not required here and would only marginally degrade the user experience in rare cases. If any changes are missed during stepping (in rare cases) and the user observes the difference they can apply the changes explicitly via Apply Changes command. Skipping a change application may lead to interesting effects in certain corner cases, but it appears to be sound.

EnC service creates a series of EditSession instances. Each instance represents state of the session in between points that change some aspect of the session (inputs to the EnC analysis) . The first instance is created when the debugging/Hot Reload session starts (F5/Ctrl+F5).

A new session is created, replacing the previous one (see DebuggingSession.RestartEditSession) at the following events:

  1. When a change is applied and committed. The new session is initialized with non-remappable regions calculated based on the applied change. The latest committed solution snapshot is also captured at this point. All further delta analysis is based on this snapshot.
  2. When a break state is entered
  3. When a break state is exited
  4. When capabilities change.

Prior to this change events [1] and [3] have been implicitly coupled. The debugger always applied changes before exiting the break state. However, we can also see them as independent. We do not necessarily need to apply changes made during break state before the break state ends. We can skip applying the changes at that point and apply them later - either while the app is running (if the user explicitly triggers Apply Changes) or at the next break state (automatically, if we detect them at that time).

All delta analysis continues to use the latest committed solution without regard of the break state. The only impact of entering and exiting break state on the EnC analysis is consideration of active statements, which are only available while at break state. It may happen that the changes were considered rude during the break state (due to presence of an active statement) while they might be allowed while the application is running. The background EnC analysis will always report the correct rude edits (squiggles) at that point. It may happen, if we skip applying a change, that the squiggle is shown while at break state, disappears when break state is exited and appears again when the app is stopped at the next break state.

Now that we determined that skipping change application during break state is sound, we can perform much faster check (heuristic) that only inspect readily available in-proc solution state. Ideally, we would avoid using solution snapshot entirely for this implementation, but that turns out to be tricky. We could, for example, track changes to open files that belong to Roslyn and ignore changes that were made outside of VS. However, determining whether a file belongs to Roslyn (i.e. can affect compilation) is not straightforward as it depends on data coming from the project system (especially for additional files, which can have arbitrary extension). In future we might want to investigate this approach further: #61692.

Fixes AB#1540395
Fixes AB#1370311
Fixes AB#1240764
Fixes #55625

@tmat tmat requested review from a team as code owners June 3, 2022 18:18
@ghost ghost added the Area-Interactive label Jun 3, 2022
@tmat tmat added this to the 17.3 milestone Jun 3, 2022
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 3, 2022

@davidwengier @isadorasophia PTAL

Copy link
Copy Markdown
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.

We do not necessarily need to apply changes made during break state before the break state ends

This is somewhat scary sounding, but also somewhat interesting. I like the idea that some rude edits become acceptable if applied outside of break state. Should we update the diagnostic message to indicate that? eg "Updates around active statements cannot be made while program execution is paused. Use Apply Changes during program execution to try again"

Alternatively, should we have a list of rude edits that this applies to, and try to automatically apply them after break state has ended, at idle or after a delay? Or is that a job for the debugger?

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jun 6, 2022

This is somewhat scary sounding, but also somewhat interesting. I like the idea that some rude edits become acceptable if applied outside of break state. Should we update the diagnostic message to indicate that? eg "Updates around active statements cannot be made while program execution is paused. Use Apply Changes during program execution to try again"

These rude edits mention "active statement" in the message (e.g. "can't edit around an active statement"). This implies that the presence of the active statement is the issue. It might not be that obvious and we could change the message a bit to make it more obvious. Feel free to file an issue to follow up.

Alternatively, should we have a list of rude edits that this applies to, and try to automatically apply them after break state has ended, at idle or after a delay? Or is that a job for the debugger?

Note that we don't pick which changes to apply and which not. All changes are either applied or not. We just choose to not apply in certain, rare cases. I don't think we should automatically apply any changes while the app is running without the customer explicitly asking us to do so. That might result in surprising behavior. Applying outside of break session (Hot Reload) has different semantics than applying when the app is stopped. Hot Reload changes are applied only to the next invocation of the method, not the current frame like EnC.

@tmat tmat enabled auto-merge (squash) June 6, 2022 18:09
@tmat tmat merged commit 7c13301 into dotnet:main Jun 6, 2022
@ghost ghost modified the milestones: 17.3, Next Jun 6, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove HasChangesAsync

4 participants