Reduce allocation in PhysicalFileProvider when used in StaticFiles #214
Conversation
| @@ -35,7 +35,7 @@ public Stream CreateReadStream() | |||
| FileMode.Open, | |||
| FileAccess.Read, | |||
| FileShare.ReadWrite, | |||
| 1024 * 64, | |||
| Math.Min((int)Length, 1024 * 64), | |||
There was a problem hiding this comment.
FileStream defines the default buffer size to be 4k. Any idea why we picked a significantly larger number? Does it make sense to stick to the 4k value?
There was a problem hiding this comment.
There was a problem hiding this comment.
The fewer round trips we can do to the disk, the better.
There was a problem hiding this comment.
What if Length is greater than int.MaxValue? I think it will overflow and would result in a value less than 0 which would also throw. https:/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/FileStream.cs#L85
long lng = (long)int.MaxValue + 1;
int i = Math.Min((int)lng, 1);
// i == -2147483648long lng = long.MaxValue;
int i = Math.Min((int)lng, 1);
// i == -1
pakrym
changed the title
|
|
pakrym
force-pushed
the
pakrym/stop-overallocation
branch
2 times, most recently
from
c71c4d8
to
c00e03d
Compare
| @@ -29,13 +29,12 @@ public PhysicalFileInfo(FileInfo info) | |||
|
|
|||
| public Stream CreateReadStream() | |||
| { | |||
| // Note: Buffer size must be greater than zero, even if the file size is zero. | |||
There was a problem hiding this comment.
Was this comment invalid? Because now Math.Min((int)Length, 1024 * 64) can return 0 which would result in an exception. Or can't Length be 0? https:/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/FileStream.cs#L85
There was a problem hiding this comment.
I thought I verified 0 length file and it worked, but I re-tested and it didn't. Thank you for noticing.
pakrym
force-pushed
the
pakrym/stop-overallocation
branch
from
c00e03d
to
35796d2
Compare
| { | ||
| var fileName = Guid.NewGuid().ToString(); | ||
| var filePath = Path.Combine(root.RootPath, fileName); | ||
| using (File.Create(filePath)) { } |
There was a problem hiding this comment.
File.WriteAllBytes(filePath, new byte[0])? A bit cleaner than the single line dispose.
|
|
1 similar comment
|
|
|
Looks good |
|
Can you do some tests with large files to see what the before/after looks like? |
|
@davidfowl why would it change for large files? |
| @@ -30,12 +30,13 @@ public PhysicalFileInfo(FileInfo info) | |||
| public Stream CreateReadStream() | |||
| { | |||
| // Note: Buffer size must be greater than zero, even if the file size is zero. | |||
| var bufferSize = (int)Math.Min(1024 * 64, Math.Max(1, Length)); | |||
There was a problem hiding this comment.
make 1024 * 64 a constant MaxBufferSize
By default we pass 64k buffer size but it causes significant overhead for small sized files.
Before:
Using 200byte file
After:
@Tratcher
@davidfowl
// Note: Buffer size must be greater than zero, even if the file size is zero.why?