Make stream buffer size configurable. #1286
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1286 +/- ##
==========================================
+ Coverage 82.67% 82.70% +0.02%
==========================================
Files 697 697
Lines 30659 30677 +18
Branches 3467 3470 +3
==========================================
+ Hits 25347 25370 +23
+ Misses 4612 4604 -8
- Partials 700 703 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
One question, otherwise looks good if @TedStryker can confirm the changes work.
There was a problem hiding this comment.
I made the BufferedReadStreamTests parametric, which makes some of them fail:
https://gist.github.com/antonfirsov/304c2a9d008446c78f786ddbd161767c
We need a reasonable minimum value.
Also fails for some weird numbers. We don't need to work with those OFC, but then we need to add checks to ensure the provided value is divisible by some pow2 number.
| get => this.streamProcessingBufferSize; | ||
| set | ||
| { | ||
| if (value <= 0) |
There was a problem hiding this comment.
Buffer size 1 and 2 will not work.
There was a problem hiding this comment.
I’ll have a look at your tests and fix up
There was a problem hiding this comment.
We're missing some guard sanitation on the Position property but the problem is really with the tests. They're expecting a minimum length.
There was a problem hiding this comment.
The main question: would Image.Load[Async] work with buffer sizes like 1, 2, 3, 503, 719 ? If not, the setter should throw.
There was a problem hiding this comment.
It'll work with any length. Was just bad tests.
|
Thx to you all, I'll do some benchmarks the next days and will keep you informed. |
|
@TedStryker thanks! It's merged, so you can use our latest nightly build now. |
|
@antonfirsov I built my own (release/x64), hope that doesn't matter. I did some more benchmarks locally and via SMB with a smaller picture (1MB) and a bigger one (4MB) and different values of StreamProcessingBufferSize. tl;drIf accessing resources via SMB (or network in general?), use .NET's default buffer size of 81920 for stream-to-stream copying (no, it's not 8192). Complete BenchmarkDotNet logs: Here are the benchmark results: BenchmarkDotNet=v0.12.1, OS=Windows 8.1 (6.3.9600.0)
Intel Core i7-4790K CPU 4.00GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
Frequency=3906254 Hz, Resolution=255.9997 ns, Timer=TSC
.NET Core SDK=3.1.400-preview-015203
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
DefaultJob : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
|
|
Gist of my benchmarks: |
|
@TedStryker Thanks. I might update our default value then. I wasn't aware the copy buffer default was as large as it was. The buffer default I used was from the |
Prerequisites
Description
Adds a new property
Configuration.StreamProcessingBufferSizewhich allows the configuration of internal buffers used to reduce synchronization costs of network streaming. The value defaults to 8096 with a minimum of 128.Fixes #1276