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

Batch FAR window update messages to lower refresh overhead. #73534

Merged
merged 3 commits into from
May 18, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 17, 2024

Drops CPu utilization in Devenv from 57Msecs in an expensive FAR scenario:

image

to just 34Msecs

image

--

While the FAR window is good, it's not good enough that thousands of notification updates dont' impact it.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 17, 2024 07:02
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 17, 2024
@CyrusNajmabadi
Copy link
Member Author

@ToddGrun ptal

// Similarly, updating the actual FAR window can be quite expensive (especially when there are thousands of
// results). To limit the amount of work we do, we'll only update the window every 500ms.
_notifyQueue = new AsyncBatchingWorkQueue(
DelayTimeSpan.Medium,
Copy link
Contributor

Choose a reason for hiding this comment

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

DelayTimeSpan.Medium

I kind of feel like I wish the ABWQ had an option where it would customize the delay as the work expanded.

Something like:
For the first 1 second, it updates on NearImmediate cadence.
Between 1 - 5 seconds, it updates on short cadence
After 5 seconds, it updates on medium cadence

It just feels like adding a 500 ms delay on really quick FAR requests might be perceptible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I def didn't notice anything feeling off here. I could make it 250ms if you feel better about that. We'll still likely get the majority of the benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like that. 250 ms is a lot less noticeable than 500 ms.

@CyrusNajmabadi
Copy link
Member Author

@ToddGrun done

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge May 17, 2024 16:33
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:

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