Use System.Memory for < .NET 5 and avoid (some) unnecessary allocations #483
Conversation
| stream.WriteByteArray(Types.Int.ToByteArray((short)(19 + (12 * amount)))); | ||
| stream.WriteByteArray(new byte[] { 0x02, 0xf0, 0x80, 0x32, 0x01, 0x00, 0x00, 0x00, 0x00 }); | ||
| stream.Write(Int.ToByteArray((short)(19 + (12 * amount)))); | ||
| stream.Write(new byte[] { 0x02, 0xf0, 0x80, 0x32, 0x01, 0x00, 0x00, 0x00, 0x00 }); |
There was a problem hiding this comment.
These byte-array allocations are avoided for newer targets.
Could be done for older targets too (by using a static readonly field), but as this code was so before I don't think we should optimize for these targets -- and for the newer ones we get this optimization for free.
| var package = new System.IO.MemoryStream(packageSize); | ||
| const int packageSize = 19 + 12; // 19 header + 12 for 1 request | ||
| var dataToSend = new byte[packageSize]; | ||
| var package = new MemoryStream(dataToSend); |
There was a problem hiding this comment.
Instead of a ToArray on the MS we can just give it our buffer and use that buffer later on.
Avoids allocations (of the MS buffer).
|
Hi @gfoidl, Sorry for acting so quickly 😆. I was considering some of the reductions you applied here, however I was very much satisfied with the fact someone else did most of the work on this. I fully agree on adding the System.Memory dependency though and making these improvements. Again your keen eye spotted some nice optimizations (the I have to look into the build environment, not sure what platform we use on AppVeyor, which is what's used to deploy the package as well. I recently did my first GitHub actions workflow to publish to NuGet (VisconFactoryIntelligence/AdsClient/blob/main/.github/workflows/dotnet.yml, which also isn't actually based on VS2022, but I guess this works out with dotnet as well). Anyway, thanks for your time and feedback, will merge and release this as well. |
Unfortunately #482 got merged too fast, so I couldn't have a look and provide my feedback over there, thus here's a PR with my suggestions 😉
For targets before .NET 5 the System.Memory package is used, which provides Span, and Memory for these targets
So the code can be simplified a bit by re-using parts of the code
internal
MemoryStreamExtension.WriteBytesrenamed toMemoryStreamExtension.Writeso that the usage as extension solely becomesstream.Write, so for .NET 5 (when built with VS 2022 as CI does ?!) the C# compiler can avoid some allocations by refering to assembly's static data segment, namely for patterns likepackage.Write(new byte[] { 0x1, 0x2, ... }. See sharplab example for this (thePrivateImplementationDetailsrefers to static data, no further allocation))