Fine tuning of what types of project update affect what state #11213
Conversation
|
Test insertion shows an improvement in the metric that previously regressed: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/592392 Note that I think that means its an improvement against the original baseline, and not the regressed metric value, so I think this is not just avoiding a hit, but actually clawing some back. Though I couldn't actually saw how, so I might be wrong about that :) |
|
|
||
| // If the host project has changed then we need to recompute the imports map | ||
| var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments; | ||
| var state = new ProjectState(this, numberOfDocumentsMayHaveChanged: !configUnchanged, hostProject, projectWorkspaceState, documents, importsToRelatedDocuments); |
There was a problem hiding this comment.
It's a bit hidden, so I'll call it out here. Where I said in the PR description that updating the DocumentCollectionVersion was the main issue, its this numberOfDocumentsMayHaveChanged parameter.
In my original PR I equated any project change to a possible document change, but tag helpers changing is definitely not that.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs
Outdated
Show resolved
Hide resolved
| @@ -160,12 +160,14 @@ public virtual DocumentState WithImportsChange() | |||
| return state; | |||
| } | |||
|
|
|||
| public virtual DocumentState WithProjectChange() | |||
| public virtual DocumentState WithProjectChange(bool cacheComputedState) | |||
There was a problem hiding this comment.
I'm still not sure when or why this should be true or false. I see in the PR but not really the impact of it
There was a problem hiding this comment.
I'll admit this isn't great, and I think the reasoning around what the computed state actually achieves, and whether its worth caching, is not fully understood by anyone. Or perhaps better said, I don't think I, or anyone else, could write a convincing comment explaining why this value should be true or false. I've made things match previous behaviour, and that seems to have had no bad outcomes, and I look forward to Dustin removing the computed state tracker in the future.
Follow up to #11191 which caused RPS regressions.
There were some edge-y things I papered over in that PR, assuming they were unnecessary over-complications, but given the RPS regression I had a closer look, and they could have caused re-compilation of closed documents when only tag helpers changed, which would be new behaviour. Not entirely convinced the re-compilation is all unnecessary (if a document used a Tag Helper that has only just been discovered, then its code gen would legitimately change) but it's hard to justify when I can't point to any tooling for the closed files that would actually benefit from the extra work.
The key thing is not changing
DocumentCollectionVersionwhen onlyProjectWorkspaceStatechanges. Not re-calculating import documents was just a little extra amuse-bouche, and not caching the computed state tracker is pure paranoia.