Reduce allocations in SourceTextStream ctor#80289
Conversation
Simple switch to use a pooled char[] as the class already has a lifetime mechanism we can hook into. Not a huge win, but it's 0.1% (2.3 MB) of all allocations in the RoslynCodeAnalysisService process in the razor speedometer test,.
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| s_charArrayPool.Free(_charBuffer); |
There was a problem hiding this comment.
Should we only do this when disposing parameter is true?
There was a problem hiding this comment.
I don't think the disposing bool is something we should key off here, but it's a good point that the code should probably be more resilient to double dispose.
There was a problem hiding this comment.
As far as I understand, disposing is false in destructor (when the object is being collected by GC) and there we cannot rely on any other objects existing (they can be freed already). I'm not sure null check is enough in that case.
There was a problem hiding this comment.
That's a good way to phrase that, I'll add a disposing check too. Someone should probably take a pass through DIspose(bool disposing) implementations, as some of them also don't use the disposing bool.
| var sourceText = SourceText.From(text, encoding); | ||
| using (var stream = new SourceTextStream(sourceText, bufferSize: text.Length * 2)) | ||
| using (var stream = new SourceTextStream(sourceText)) | ||
| { |
There was a problem hiding this comment.
How do we know that this change doesn't reduce test coverage? Could it be that the test scenario is not setup properly now? #Closed
There was a problem hiding this comment.
I'll take a closer look and make sure the coverage isn't reduced.
There was a problem hiding this comment.
This test is a little tricky. I think my confusion lied on whether it was validating whether the buffer it allocated was being filled properly, or whether the buffer SourceTextStream allocated was being refilled properly. I think you're right and that it's the latter.
I've modified the test such that baseText is now one shorter than the array that SourceTextStream uses to match the old behavior.
There was a problem hiding this comment.
It looks like this test was added in 6313aa3. Please confirm that the modified test is failing if the fix is undone.
There was a problem hiding this comment.
Left tab: Local reversion of the changes in the commit you pointed too
Middle tab: New test contents
Right tab: Test explorer window validating test failiure
| if (_charBuffer != null) | ||
| { | ||
| s_charArrayPool.Free(_charBuffer); | ||
| _charBuffer = null!; |
There was a problem hiding this comment.
I was more concerned about keeping a reference to _charBuffer, as then any usage of it by the class would then be operating on something that had already been returned to the pool, and that's hard to track down. If you don't like the setting of it to null, we could instead use a bool disposed field and check that before all usage of _charBuffer.
There was a problem hiding this comment.
we could instead use a bool disposed field and check that before all usage of _charBuffer.
And throw an exception? How is that different? An existing code might still break.
There was a problem hiding this comment.
To be clear, I don't think that adding a bool _disposed field is warranted, and I prefer setting _charBuffer to null.
If _charBuffer is used while null, that would mean that someone is using this stream after it has been disposed. I think that warrants an exception, but if you are wary of throwing an exception in that scenario, then a bool field could be added to avoid that exception at use sites of _charBuffer.
There was a problem hiding this comment.
It is not clear to me what you are suggesting to do. The change, as it stands right now, affects behavior of the component beyond just changing an allocation strategy. We might discuss what should/could be the safe usage pattern for the component and might even agree on that, but that wouldn't have any effect on the way this component is already used. When I see a PR with a title "Reduce allocations in SourceTextStream ctor". I generally do not expect to see any observable behavior changes.
There was a problem hiding this comment.
If we can agree on that the code could generally throw if used after disposal, and we can validate that no code currently does that and so doing that doesn't break any existing assumptions, would that be sufficient?
There is only a single usage of this class in product code, and that code disposes the stream and makes no access to it after that point.
There was a problem hiding this comment.
If we can agree on that the code could generally throw if used after disposal, and we can validate that no code currently does that and so doing that doesn't break any existing assumptions, would that be sufficient?
I think it would.
There was a problem hiding this comment.
There is only a single usage of this class in product code, and that code disposes the stream and makes no access to it after that point.
This addresses my concerns
|
Done with review pass (commit 2). At a glance, the change looks risky to me. #Closed |
2) Modify test back to original intent
|
Done with review pass (commit 3). Waiting on a confirmation regarding the modified test. #Closed |
4 participants
Simple switch to use a pooled char[] as the class already has a lifetime mechanism we can hook into.
Not a huge win, but it's 0.1% (2.3 MB) of all allocations in the RoslynCodeAnalysisService process in the razor speedometer test,.