Make GetDocumentWithFrozenPartialSemantics asynchronous #57830
Conversation
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/SpecializedTasks.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Please update the PR explaining what the root cause was here -- it's not clear to me why making this async versus fixing the lock is the correct choice here. (The expectation is the code under the lock is cheap, clearly the bug implies it's not but something is amiss.)
|
@jasonmalinowski It's not clear to me how the operation under the lock was expected to be fast. Here's the problematic stack showing I/O under the lock (
|
sharwell
force-pushed
the
async-frozen-semantics
branch
from
5678c32 to
98cf30b
Compare
|
Doing some investigation with @sharwell, it appears that asking a Compilation for trees forces the declaration tree to be realized which is most unexpected. Either it seems we should try to optimize the compiler directly (since all sorts of things can ask for trees of a compilation and nothing is unique about the frozen logic), or change FreezePartialStateWithTree to use the ExternalSyntaxTrees directly which looks like it's not a computed value. |
sharwell
force-pushed
the
async-frozen-semantics
branch
from
6db7f4a to
c154315
Compare
src/Compilers/CSharp/Portable/Declarations/DeclarationTableInput.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Declarations/DeclarationTableInput.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Declarations/DeclarationTableInput.cs
Outdated
Show resolved
Hide resolved
sharwell
force-pushed
the
async-frozen-semantics
branch
from
c154315 to
98cf30b
Compare
| @@ -24,10 +22,10 @@ public DocumentProvider(IThreadingContext threadingContext) | |||
| { | |||
| } | |||
|
|
|||
| public Document GetDocument(ITextSnapshot snapshot, CancellationToken cancellationToken) | |||
| public ValueTask<Document?> GetDocumentAsync(ITextSnapshot snapshot, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
terrifying that thsi doesn't state taht it's getting frozen partial. i know that's what it was before, but i am genuinely baffled by this. also, do we even need this type? any caller can just do the code below right?
| @@ -24,10 +22,10 @@ public DocumentProvider(IThreadingContext threadingContext) | |||
| { | |||
There was a problem hiding this comment.
why is this a FG object?! i don't get this at al :)
| { | ||
| get | ||
| { | ||
| // TODO: why did I need to do a nullable suppression here? | ||
| return LazyInitializer.EnsureInitialized(ref _stateLockBackingField, NonReentrantLock.Factory)!; | ||
| return LazyInitializer.EnsureInitialized(ref _stateLockBackingField, () => new SemaphoreSlim(1))!; |
There was a problem hiding this comment.
| return LazyInitializer.EnsureInitialized(ref _stateLockBackingField, () => new SemaphoreSlim(1))!; | |
| return LazyInitializer.EnsureInitialized(ref _stateLockBackingField, static () => new SemaphoreSlim(1))!; |
|
So i'm ok with this in teh current state. @jasonmalinowski do you still have objections here? This approach appears to be overall a fairly safe win without dramatic design changes. The existing code synchronous code that moves to JTF seems to be no worse to me than how it was before where it had to block anyways on the semaphore. |
|
@CyrusNajmabadi Indeed, the making of frozen partial semantics should not be async here, fundamentally because that operation is supposed to be fast. It should not be doing expensive computational work, and shouldn't really be blocking on other things. Once we dug with @sharwell further we discovered the "actual" issue in my book was that the compiler had an operation that was really expensive that simply had no reason to be. There's also some locking that we're taking in this path for a really, really suspicious cache which we should just get rid of. The fact this wasn't async was actually very helpful when implementing support for source generators and open files, since it made it very clear to me that I could not put anything expensive on this path. There was more than once it was really tempting to go run generators but since that was fundamentally an async operation it forced me to reconsider what I was trying to do. Put another way, the lack of async here is a feature: it forces you to figure out what your actual problem is! 😄 |
|
And I won't say that we may not have a point at some point in the future where it makes sense; at least for the thing that triggered this change, this was the wrong fix; by not taking this it forced us figure out where we can make better fixes in other parts of the system. But this being async means it's easy to not root-cause a perf analysis and just "make something more async" when there's further investigation to do. |
|
SGTM, thanks for the explanation. |
|
Closing this as Won't Fix on the IDE side, and reassigned the original perf epic bug to compilers. |
sharwell
added
the
Resolution-Won't Fix
In the context PerfEpic bug AB#1436225 ([Resposiveness] WithFrozenPartialCompilationIncludingSpecificDocument blocking large number of threads with a synchronize wait), I found that the compiler was performance a significant amount of work inside
SyntaxAndDeclarationManager.GetLazyState(). This led to calls to obtain frozen partial semantics holding a lock for longer than expected. I implemented two solutions (one in IDE, one in compiler), where we can take either or we can take both. The IDE resolution eliminates the assumption that obtaining frozen semantics is fast enough to ensure lock contention is not a problem by makingGetDocumentWithFrozenPartialSemanticsasynchronous (SemaphoreSlimcan be obtained asynchronously). The compiler resolution splitsGetLazyState()into a two-stage lazy value, with most items computed in the call toGetLazyState()but the declaration table (expensive when syntax trees are not already in memory) computed via a second layer call.@jasonmalinowski (per #57830 (comment)) is in favor of fixing this only in the compiler, since the current synchronous signature of
GetDocumentWithFrozenPartialSemanticsis itself a hint that the API is not expensive and that is a key design characteristic/goal of this path.The former approach can be seen in this pull request. The second approach has been submitted in #59392 for the compiler team to optionally move forward.
Fixes AB#1436225