Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the 'Compilation Available' tagger event source #72384

Merged

Conversation

CyrusNajmabadi
Copy link
Member

This system doesn't work in the current 'frozen snapshot' model that the workspace has moved to. THe workspace's current model is that when you ask for a frozen partial (FP) snapshot off of hte 'current' snapshot, that that FP snapshot is computed and cached so that you get the same one in the future for all further callers (until the workspace's CurrentSolution is actually updated).

We also updated FP snapshots so that they will always benefit from any parsed files in the projects (previously, the projects had to parse all files until you would see any of them). These changes made things much simpler, and meant we both weren't recomputing work and we were benefiting from work that had been done (including for future FP snapshots).

However, this changes things in that some features basically hardcoded in an idea that they would just 'try again' at a later point, with the belief that they would then go 'better' results since things would have changed underneath them with the same original snapshot. But that does not hold, meaning there was just extra complexity (esp. on a cpu level) for no actual benefit.

Practically, this is not likely to matter much in practice. That's because:

  1. we already cache full classification from the prior session which we persist to disk.
  2. we parse documents in parallel, and preserve information about any documents we parsed when getting the frozen snapshots.

So the time period to observe incorrect results is now narrower. Given that this wasn't working anyways, and that the case where this matters is far narrowed, the code has now been removed for simplicity.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 4, 2024 18:21
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 4, 2024
private void OnUnderlyingSourceChanged(object? sender, TaggerEventArgs args)
{
// First, notify anyone listening to us that something definitely changed.
this.Changed?.Invoke(this, args);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.Changed?.Invoke(this, args);

I think I'm clear on not notifiying listeners on frozen partial compilations, but would it make sense to still notify them when the compilation has actually reached a final state?

Copy link
Member Author

Choose a reason for hiding this comment

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

so the main problem is simply that we'd need those features to have a new concept (Which doesn't exist at all). Which is:

  1. use the compilation if available, but otherwise, use teh frozen partial one.

Note that the above is highly problematic as well as the features will often be working with a slightly changed doc, and thus are calling "get frozen partial snapshot with changes". In that world, we def do not want anything related to the real compilation as that involves computing generators.

This basically also involves the workspace having some sort of internal eventing model itself for these sorts of concepts. I'm very wary of that, esp. as it encourages more of a 'push' approach for these features.

My preference for now is simply saying: if you use frozen partial, you are opting into staleness against the snapshot you're currently operating on.

@@ -48,12 +48,7 @@ private sealed class Tagger : IAccurateTagger<IClassificationTag>, IDisposable
_subjectBuffer = subjectBuffer;
_globalOptions = globalOptions;

// Note: because we use frozen-partial documents for semantic classification, we may end up with incomplete
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised that a search for "frozen" didn't hit in this document. Just wanted to make sure we are not missing a WithFrozenPartialSemantics call here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it happens in the classification service itself. that said, you raise a good point that it's odd for it to happen there. i will look into having the underlying helpers not do that, and instead bring to the feature level.

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit afe03b2 into dotnet:main Mar 5, 2024
27 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the removeCompilationEventSource branch March 5, 2024 04:53
@sharwell
Copy link
Member

sharwell commented Mar 5, 2024

I have a lot of concerns that this is breaking core functionality in Roslyn. By removing this necessary (but currently broken?) system, we are now in a position where we not only have to fix it, but we also have to go back and find all the places to add it back.

@CyrusNajmabadi
Copy link
Member Author

I discussed with Todd potential options that actually would be correct from a design perspective. If we wanted to add them back in, it would not be difficult to do. I don't see the point in keeping expensive and non functional code around. No one is helped by that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants