Use a known buffer size to reduce allocations#2306
Use a known buffer size to reduce allocations#2306allreadyexists wants to merge 1 commit intodotnet:mainfrom
Conversation
|
@dotnet-policy-service agree |
idg10
left a comment
There was a problem hiding this comment.
In addition to the comments in this review, I've got a question: have you done any experiments to show the impact of this on allocations and performance?
E.g., I can think of two scenarios in which this will actually have worse allocation characteristics than the current implementation. (One is slightly contrived: it's the case where the buffer size happens to be 4. In that scenario you get no benefit from specifying a capacity because List<T> defaults to 4 anyway, but you've now made Ferry larger than it needs to be because it's got an extra 4 bytes for that _count. But the other is more plausible: if a source completes without filling its buffer, we emit the final non-full buffer, and with this change, the space reserved for that is larger than it needs to be.)
When I work to improve allocation characteristics of libraries, I've generally found that testing reveals factors I didn't think about, so the two possibilities I've thought of above are probably not the only ones.
|
|
||
| protected override void Run(_ sink) => sink.Run(); | ||
|
|
||
| #if NET8_0_OR_GREATER |
There was a problem hiding this comment.
The conditional compilation here is at what seems like an unnecessarily coarse level. It makes it very hard to see what's actually different between the #if and #else arms.
As far as I can tell there are only 3 lines that are different: 1) you've added a new _count field, 2) you've used that to reserve capacity when initializing _s and 3) also when re-initializing it after a buffer fills.
If I have missed other changes, then that illustrates my point.
I would want the conditional compilation directives to wrap only those parts that are different, because otherwise, anyone maintaining this code is forced to read a lot of code to try to deduce why there are two different versions.
The unnecessary duplication also makes it more likely that if bugfixes or further performance enhancements are made in this code, that they are accidentally applied only to one of the two copies of the code.
| newId = ++_windowId; | ||
|
|
||
| var res = _s; | ||
| _s = new List<TSource>(_count); |
There was a problem hiding this comment.
If I understand correctly, this is basically the whole point of this change: it sets the capacity of the List<TSource> so that the list will never have to reallocate as the buffer contents grow.
If that's right then I don't understand why you made this change apply only to .NET 8.0 or greater. As far as I can remember that constructor has been available since .NET Framework 2.0 (i.e., for over 20 years). It's available in netstandard2.0, net472, and uap10.0.18362, i.e. all of the frameworks we target.
In dotnet 8 and above, you can use a fixed-size buffer to reduce the number of allocations and copies