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

Revert "Move workspace file removal to BG to avoid UI delay" #59241

Closed
wants to merge 1 commit into from

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Feb 3, 2022

@sharwell
Copy link
Member

sharwell commented Feb 3, 2022

Can you also mention the RPS counter that potentially will be fixed by this?

@CyrusNajmabadi
Copy link
Member

Why are we reverting this? This addresses a severe UI hang (think 10s of seconds).

@RikkiGibson
Copy link
Contributor Author

I apologize that I wasn't able to communicate what was going on here at the time of opening the PR.

I'd like to emphasize that reverts of your PRs by infraswat are never intended to mean that your change is rejected, and they are never meant to say your change doesn't do something important and useful. When a release branch gets blocked, it is infraswat's job to unblock it as soon as possible. Since validating the RPS behavior of any change takes several hours of waiting for CI, we will queue a revert of something even if there's a low chance that reverting that thing is the fastest way to unblock the release branch.

This revert was part of investigating RPS failures in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/377533. Failing counter is BuildInfoVS64.Test.Build_Ngen_InvalidAssemblyCount (increased by 1).

Unfortunately, we still don't have per-PR granularity of insertions. This means that when I need to track down which PR is responsible for a regression, I generally have to look at the set of PRs between the last-known-good and first-known-bad insertion. In this case, that is:

Handle nullable annotations on user defined operators and conversions (59169)
Refactor ISyntaxInputNodes (58800)
Remove dependency on EditorFeatures from Remote.ServiceHub project - take 2 (59147)
Update 17.1 (59153)
Move workspace file removal to BG to avoid UI delay (59211)

Here I found 2 PRs to be suspicious: #59147 and #59211. By pure speculation I opened revert PRs #59241 and #59242 for these on GitHub and kicked off VS validation runs for those reverts. Just by doing this I can gather evidence for which one is actually causing the regression. This was all I was able to do to triage the failures this morning since I didn't have a lot of time.

Infraswat work is often a matter of kicking off jobs that take several hours speculatively just in case it ends up providing critical information we need in order to unblock our releases. We also have Mantis go into the BRAT tool and figure out exactly what the regression is (i.e. what new assembly is loading and when and why). That said, we normally want to also contact authors whose changes we suspect introduced regressions. I'll ensure that this communication happens up-front in the future.

@sharwell
Copy link
Member

sharwell commented Feb 4, 2022

@RikkiGibson I didn't mean to question the strategy, just wondering if it was a perf item or one of the tedious assembly load counters. Looks like it's the latter.

@RikkiGibson RikkiGibson closed this Feb 4, 2022
@RikkiGibson RikkiGibson deleted the revert-59211-bgFileRemoval branch February 4, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants