Avoid recomputing generated sources for the same input#50172
Avoid recomputing generated sources for the same input#50172sharwell merged 5 commits intodotnet:masterfrom
Conversation
jasonmalinowski
left a comment
There was a problem hiding this comment.
Seems there's some cleanup opportunity; I admit I'm also hesitant that the cache here is a static because it means unloading the generator is a memory leak where we're leaking the rest of the documents, and since the SourceText is held strongly it's not a trivial leak either. I was expecting we'd just do a ConditionalWeakTable keyed on the AdditionalFile or something similar, since I think we had reasonable identity currently working in the system?
There was a problem hiding this comment.
Can this either have some documentation or use a named tuple to explain what the ImmutableArray<byte> is here?
There was a problem hiding this comment.
Think we need at least a doc comment explaining that this value is expected to be used from multiple threads
There was a problem hiding this comment.
It looks like each place is being used exactly once; do we need this enum and all the fancy switching? Seems like this would greatly simplify the code. "we'll use it later" doesn't pass muster for me here since we're currently working on API proposals to eliminate the need to do this.
There was a problem hiding this comment.
(and there's still other writing happening directly in on line 59. Just inlining the cache updating would greatly simplify this code.)
There was a problem hiding this comment.
Inlined/simplified
There was a problem hiding this comment.
Think we need at least a doc comment explaining that this value is expected to be used from multiple threads
jasonmalinowski
left a comment
There was a problem hiding this comment.
On second thought we should address the leak problem before this goes in -- if the user unloads a solution and then loads something else, the static is going to root megabytes of stuff...forever. If nothing else, that's not going to make leak analysis any easier.
Since this is an opportunistic cache, how about just switching to |
616a36a to
f5ccedb
Compare
|
@jasonmalinowski this is now updated |
f5ccedb to
6998f28
Compare
jasonmalinowski
left a comment
There was a problem hiding this comment.
So change looks good other than that comment that says SetTarget is probably thread-safe. I'm not seeing that supported in documentation. The implementation does look like it's taking a lock here:
But that's an explicit lock which means the comment is wrong for the reason it's safe.
There was a problem hiding this comment.
I would assume we can find a stronger statement about the thread-safety here? Given my understanding is the WeakReference type creates a GC Handle and has a finalizer to free said handle, that this isn't just a simple pointer assignment.
There was a problem hiding this comment.
Also, the SetTarget acts a memory barrier which ensures all writes are complete first?
There was a problem hiding this comment.
Also, the SetTarget acts a memory barrier which ensures all writes are complete first?
This doesn't matter. The only safety we need is atomicity.
I verified with @stephentoub that the type is thread safe and am opening a doc bug for it.
There was a problem hiding this comment.
➡️ Updated the comment, and filed dotnet/dotnet-api-docs#5276 for the documentation bug
6998f28 to
2c29a9c
Compare
Update
CSharpSyntaxGeneratorto return the sameSourceTextinstance whenExecuteis called but the input definition has not changed.This is immediately beneficial as a standalone change, but produces larger wins in combination with #50171.
For the source generator in Roslyn.sln, this change addresses ~35% of the overhead for repeated invocations of the source generator.