-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Skip updating signature-help if there's another update request in the update-queue. #17772
Skip updating signature-help if there's another update request in the update-queue. #17772
Conversation
Tagging @dotnet/roslyn-ide |
@Pilchie Can you try this with your scenario where you saw SignatureHelp slowdowns? |
…ilter out a command that has data we might need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem bad, but isn't this what those cancellation tokens are for? Why not just cancel the chain rather than multiple ways of cancellation?
/// we keep track if there is new outstanding retrigger command and we bail on the | ||
/// computation if another is in the queue. | ||
/// </summary> | ||
private int _retriggerId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be marked volatile? I can never remember.
(triggerInfo.TriggerCharacter.HasValue && !currentModel.Provider.IsRetriggerCharacter(triggerInfo.TriggerCharacter.Value))) | ||
if (currentModel == null) | ||
{ | ||
return currentModel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also known as "null".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
/// previous model we've computed. | ||
/// </summary> | ||
private async Task<(ISignatureHelpProvider provider, SignatureHelpItems items)?> ComputeItemsAsync( | ||
int id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this retriggerId or something so it's clear it relates to the other field? id
is a bit vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically duplicating the pattern used in http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Intellisense/Completion/Controller.Session_FilterModel.cs,63002199f3349dd6 . Any way we could push the filterId stuff up into the ChainModelAndNotify... infrastructure?
@rchande The issue is that skipping is very scenario specific. You only want to skip for certain types of messages, when you know it's totally safe to do so. For example, you can't skip a retrigger that is based on a typed-char, because the next retrigger might be for a different character which may lead to different results. |
Finally gave this a try with release bits today and it definitely feels much better! Thanks @CyrusNajmabadi |
No description provided.