Updating some readonly static data in JpegEncoderCore to take advantage of compiler functionality. #855
Conversation
|
|
Before:
After:
|
Codecov Report
@@ Coverage Diff @@
## master #855 +/- ##
=======================================
Coverage 88.89% 88.89%
=======================================
Files 1014 1014
Lines 44295 44295
Branches 3208 3209 +1
=======================================
Hits 39376 39376
Misses 4198 4198
Partials 721 721
Continue to review full report at Codecov.
|
| // This is a port of the CoreFX implementation and is MIT Licensed: https://github.com/dotnet/coreclr/blob/c4dca1072d15bdda64c754ad1ea474b1580fa554/src/System.Private.CoreLib/shared/System/IO/Stream.cs#L770 | ||
| public static void Write(this Stream stream, ReadOnlySpan<byte> buffer) | ||
| { | ||
| byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length); |
There was a problem hiding this comment.
We usually prefer using our own memory management primitives like MemoryAllocator.AllocateManagedByteBuffer() instead of ArrayPool.Shared.
There was a problem hiding this comment.
This is matching a signature and implementation available in .NET Core. ArrayPool<byte>.Shared should generally be allocation-less for something like this (not necessarily for other T) since most of the framework is fairly dependent on it.
I'll run benchmarks on net472 as well though, in order to see what the overhead here is, if any, since it isn't applicable to netcoreapp2.1 (where the above benchmark was run).
There was a problem hiding this comment.
Our "allocator" is typically also an allocation free pool, but we definitely have an overhead because of nested virtual calls, and other infrastructure stuff.
Everything depends on the size of the array. It's not uncommon to have very large (> 1 MB) buffers in image processing, which works better with our allocator/pool in my experience.
However, for those buffers we should avoid invoking this extension method anyways. @JimBobSquarePants we can probably make an exception here, but we need to make sure the purpose is documented, or at least we remember it 😄
There was a problem hiding this comment.
Definitely and I don't recall if benchmark.net isolates each iteration or not (I know it does for separate tests), so it could be that the benchmark is "lying" for full framework and it is hiding the allocation cost (so it might not be representative of real world scenarios).
It would be possible to take a MemoryAllocator as an optional parameter or to not have SosHeaderYCbCr take advantage of this compiler optimization (the single copy on class instantiation isn't terrible, but losing out on permanent pinning is a bit unfortunate) or to only not use the optimization on full framework if there are concerns about this regressing full framework.
IIRC, the ArrayPool<byte>.Shared is optimized for up to 2MB, since we use it for things like the JSON and XML readers.
There was a problem hiding this comment.
Let's add a comment explaining why we use that pool (signature matching) and move on.
There was a problem hiding this comment.
Fixed. Also rebased onto the current HEAD.
| byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length); | ||
| try | ||
| { | ||
| buffer.CopyTo(sharedBuffer); |
There was a problem hiding this comment.
To me it looks like we are loosing all the benefits of the copy-free initialization at this line...
There was a problem hiding this comment.
But the benchmarks show different results ... I wonder why? I guess this method is not a hot path.
There was a problem hiding this comment.
In the case of netcoreapp, the method is actually virtual and various stream types can override it to avoid the copy. I'll double check the numbers on full framework to see if it hurts anything (and if so, will look at what can be done).
Before:
After:
|
…ge of compiler functionality.
|
Merging this in. Thanks @tannergooding |
4 participants
Prerequisites
Description
This is a partial fix of #854 and shows how to take advantage of the underlying compiler functionality.