Tweak delays we have before we start incrementally analyzing in OOP.#46426
Tweak delays we have before we start incrementally analyzing in OOP.#464266 commits merged intodotnet:masterfrom
Conversation
| // for primary workspace in OOP to be synced to latest. | ||
| public static readonly Option<int> SolutionChecksumMonitorBackOffTimeSpanInMS = new Option<int>( | ||
| nameof(InternalFeatureOnOffOptions), nameof(SolutionChecksumMonitorBackOffTimeSpanInMS), defaultValue: 4000, | ||
| nameof(InternalFeatureOnOffOptions), nameof(SolutionChecksumMonitorBackOffTimeSpanInMS), defaultValue: 500, |
There was a problem hiding this comment.
Is it possible that this regresses other experiences? For example, now that we much more aggressively push new snapshots to OOP, any other OOP requests, say active file diagnostics request from a subsequent edit, get delayed, taxing the whole system? As in, instead of making OOP computations and sync happen more often, it is better to work towards a pull model where any analysis/compute that needs to be responsive should have an inproc component (either in Roslyn or above the stack) that pulls on this data?
There was a problem hiding this comment.
i used this for a couple of hours today and didn't feel like anything was regressed.
For example, now that we much more aggressively push new snapshots to OOP, any other OOP requests, say active file diagnostics request from a subsequent edit, get delayed, taxing the whole system?
I technically don't think this is an issue. here's why:
First, syncing this stuff is not expensive. i.e. we're literally saying: if anything changed, just push that over in the next half second. It's extremely difficult for that to be a lot of work as effectively we only have one changes were possible in that time frame.
Second, when requests are actually explicitly made for some snapshot, that syncing has to happen anyways. And syncing is complimentary. i.e. when the 'active file diagnostics' request goes in and asks to sync the snapshot, it benefits from the preexisting pushed information that we already started on. even if the particular file it is asking for has changed, it still gets all the other changes pushed over, and only really needs to potentially just sync over it's very latest text change.
it is better to work towards a pull model where any analysis/compute that needs to be responsive should have an inproc component (either in Roslyn or above the stack) that pulls on this data?
Yes, i believe that's a better long term approach. This is intended to put us in a better short term position though until partner teams can work with us on an appropriate 'pull API' here.
| new IncrementalAnalyzerProviderMetadata( | ||
| nameof(RemoteDesignerAttributeIncrementalAnalyzerProvider), | ||
| highPriorityForActiveFile: false, | ||
| highPriorityForActiveFile: true, |
There was a problem hiding this comment.
This change is no longer relevant right?
There was a problem hiding this comment.
it still is for me, eve if it's not respected. i.e. if our OOP crawler can support this concept at some point, we would want this prioritization for these files. It will mean that changes to them are realized in the UI at least a second faster, which would be preferred over the current approach. i.e. we'd likely get down to about 1-1.5 seconds from teh current 2-2.5 seconds.
|
Note: i've been running with this all day long, and things feel fine for me. I can't really perceive any issues here with anything feeling worse/better after my change. |
| writer.WriteString(data.Category); | ||
| projectVersion.WriteTo(writer); | ||
| } | ||
| } |
There was a problem hiding this comment.
missed removing this from previosu PR.
|
Hello @CyrusNajmabadi! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@CyrusNajmabadi I am in favor of this change, but I would like a follow up test plan for how we are going to track the user experience here. Right now checking it in and dogfooding seems to be the only way to verify this. I don't think we should block this PR, but I would feel more comfortable if we had some detailed thoughts about how to track the experience here for all users. |
|
@jmarolf did you have something in mind? |
|
well the question we want to answer is "How long does it take for an update to round-trip through OOP for users". So I think telemetry + a dashboard would be a reasonable first approach. It may be that we need to develop a more complete acceptance test for the round-trp experience that can run on CI but that could probably happen later. Is the question I've identified a correct way to think about the experience though? |
|
If we start monitoring round-trip times, it would be really interesting to put in a progressive backoff here. If load is low, latency is near-instant. But if load is high then we increase the latency a bit. |
Followup to #46391. That should be reviewed and merged first.'
Our existing OOP system works in the following manner:
All in all, this leads to a situation where there can be a minimum of around 7.5 seconds between a text change, and a corresponding VS UI update. This is very noticeable and makes the product feel broken.
The above worked well because we didn't use to do things proactively in OOP. i.e. OOP was a place where requests were remoted in response to an explicit user request (i.e. user explicitly invokign FAR). At that point, we'd just need to sync a little data (fast), and then perform our op and return the results.
Now that we have proactive OOP subsystems running automatically in response to changes, the above isn't viable as it just takes far too long before the work even kicks off. I've changed the above to now be:
Things brings our time delay for things like updating the winforms designer icon from 7+ seconds down to a much more reasonable 2.25 seconds. This does not feel immediate, but feels much more reasonable. Instead of taking so long that it feels like something is actually not working, things happen fast enough that you can move to your next task.