Avoid producing a new tag tree for buffers that aren't part of the SpansToTag#27933
Avoid producing a new tag tree for buffers that aren't part of the SpansToTag#27933sharwell merged 4 commits intodotnet:masterfrom
Conversation
There was a problem hiding this comment.
this is extremely scary to me. I would really want to understnad when/how could this happen. How do not have any spans we want to tag that match the buffer of the we just generated? that seems like this is masking a bug happening elsehwere...
There was a problem hiding this comment.
I agree. At first I pursued two other approaches:
- Avoiding this method if
spansToTagis empty (turns out it was not empty) - Using
cancellationToken.ThrowIfCancellationRequestedif no matching spans were found (cancellation was not requested)
I was not able to determine how/why we enter this state, but it's causing a spike in Watson since last week and our integration tests are consistently failing.
There was a problem hiding this comment.
can we NFW at least. this feels bad to me.
Note: a better approach might be to have this data just passed through the API instead of trying to re-establish it at this point in the code. It might make it clearer at which point the info is getting 'out of sync' as it were...
There was a problem hiding this comment.
Is this already creating fatal exceptions? Do we have dumps for those?
There was a problem hiding this comment.
But I agree with @CyrusNajmabadi -- this is masking a very bad problem. Let's only do this if we have a clear plan in place for how to address it further.
There was a problem hiding this comment.
Note: this is also pretty scary to me given that taggers are supposed to be either view or buffer associated. If we're breaking that assumption... then we need to do some pretty substantial rethinking of things.
The special case here is the multi-buffer tagging we do for reference highlighting: we have a single one of these for an entire view, where we're tagging each of the subject buffers in the view. This was the explicit new design when we did this refactoring originally.
There was a problem hiding this comment.
Going back to the "why?" here. Reference highlighting in interactive is tricky. It can't be a traditional buffer tagger, because you might be tagging a buffer (i.e. a submission) but the caret can be in another buffer. If we make it a view buffer that tags the surface buffer, you end up with other interestingness:
- This means our tagger is tagging the surface buffer directly, and who knows what content type that is.
- We now get to sort out what is going on with projection buffers as edits happen and make sure we are still giving sane tags.
There was a problem hiding this comment.
Ok. if it's explicitly intentional that we have a single tagger for a collection of buffers, then it feels to me liek we then need to make the internal data-types more explicit about what they are assocaited with. i.e. instead of just getting a random set of tags, you get back a set of tags-for-a-particular-buffer. And the entire stack here is designed around appropriately query and managing the information on a per-buffer level.
There was a problem hiding this comment.
Other than the first line in ProcessContext I think what you want Cyrus is what we have; this bug here is just a bug actually handling the data structures, or at least handling them poorly. I think what happened were was just some refactoring that didn't understand the 'why' before it was refactored.
There was a problem hiding this comment.
Or put another way: we could change the API interaction, and still have this bug. 😄
jasonmalinowski
left a comment
There was a problem hiding this comment.
This seems like the right approach, but I suspect we can delete a few lines of code now.
There was a problem hiding this comment.
Shouldn't we be updating the computation of buffers in line 392 or so instead? It seems like you can just delete that and make this loop over buffersToTag instead?
(I'm guessing the spansTagged.Select(dss => dss.SnapshotSpan.Snapshot.TextBuffer) is the equivalent of your buffersToTag hence this seems we're adding stuff only to filter it back out again.)
There was a problem hiding this comment.
Jeez, i hate like 382 :). Why not just require a realized list...
There was a problem hiding this comment.
➡️ This was substantially simplified in b9e81c9. Rationale for the individual changes is detailed in the commit message.
|
@sharwell Are you planning on excising the original commit from the PR? It's now superseded, but would probably confuse any future code archeologist. |
* `buffers` is an unnecessary intermediate variable. Any text buffer present in `buffersToTag` but not also present in `buffers` will end up ignored by `ComputeNewTagTree`, and thus not influence the result of `ConvertToTagTrees`. Based on this, `buffers` was eliminated to simplify the implementation. * When `spansTagged` has only a single span, it's possible that other tag trees in `oldTagTrees` need to propagate forward. The early return failed to account for this, and if hit would drop `oldTagTrees` tags for buffers not equal to the buffer from `spansTagged`. In addition, the early return failed to account for the case where `spansTagged` only contained a single span from a buffer not requested per `buffersToTag`. Following the elimination of `buffers`, the value of a special case handling here was notably reduced and the edge case was simply eliminated in favor of the remaining code in the method. * The patter of Select followed by ToLookup was replaced by a single call to ToLookup specifying both the keys and the values of the resulting lookup.
9116df7 to
b9e81c9
Compare
It is now eliminated. |
jasonmalinowski
left a comment
There was a problem hiding this comment.
The code seems both functionally correct and simpler, which is the best kind of change. I suspect we might be losing a perf optimization that the old code did, although I can't vouch that the optimization was actually good.
| // common case where we only tagged a single range of a document. | ||
| if (spansTagged.IsSingle()) | ||
| { | ||
| return ConvertToTagTree(oldTagTrees, newTagsByBuffer, spansTagged.Single().SnapshotSpan); |
There was a problem hiding this comment.
[Questions which can be followed up later:]
I believe ConvertToTagTree is now dead? Should ComputeNewTagTree still call the special case overload if spansToInvalidateByBuffer gives buffer with a single span?
There was a problem hiding this comment.
I could go either way. For now the code is updated to eliminate all the code which is now dead.
|
@jinujoseph requesting approval to merge. |
|
Well i love how red this PR is :) |
|
Approved to merge for 15.8.Preview4 |
| foreach (var buffer in buffers) | ||
| foreach (var buffer in buffersToTag) | ||
| { | ||
| var newTagTree = ComputeNewTagTree(oldTagTrees, buffer, newTagsByBuffer[buffer], spansToInvalidateByBuffer[buffer]); |
There was a problem hiding this comment.
so we basically recalculate all tags every time and never tries to reuse some? is that okay with the view port only tagger thingy?
|
Note: i will try to review this by EOD. |
Fixes recent integration test failures in
CSharpReplIdeFeatures.Here is an event log entry observed prior to correcting this:
@ivanbasov The Watson bucket for this failure is b0ec2240-0187-3815-8eed-503f273d8589.
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.