Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use RecyclableMemoryStream #107

Merged
merged 2 commits into from
Aug 25, 2020
Merged

Use RecyclableMemoryStream #107

merged 2 commits into from
Aug 25, 2020

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 馃懏.

A replacement for #106 that uses RecyclableMemoryStream for buffer pooling.

Note. Suggestions welcome for default pooling configuration values.

@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #107 into master will increase coverage by 0.82%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   77.19%   78.01%   +0.82%     
==========================================
  Files          46       45       -1     
  Lines        1324     1128     -196     
  Branches      211      174      -37     
==========================================
- Hits         1022      880     -142     
+ Misses        238      191      -47     
+ Partials       64       57       -7     
Flag Coverage 螖
#unittests 78.01% <100.00%> (+0.82%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...rc/ImageSharp.Web/Commands/ComparableExtensions.cs 16.66% <酶> (酶)
...s.Azure/Providers/AzureBlobStorageImageProvider.cs 78.72% <100.00%> (+1.45%) 猬嗭笍
...s.Azure/Resolvers/AzureBlobStorageImageResolver.cs 92.30% <100.00%> (+2.30%) 猬嗭笍
...ndencyInjection/ImageSharpCoreBuilderExtensions.cs 78.12% <100.00%> (+2.26%) 猬嗭笍
...DependencyInjection/ServiceCollectionExtensions.cs 86.95% <100.00%> (+0.59%) 猬嗭笍
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs 71.42% <100.00%> (-1.06%) 猬囷笍
...harp.Web/Middleware/ImageSharpMiddlewareOptions.cs 57.69% <100.00%> (+1.69%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update a11d088...da5f5e7. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and we are likely fine with the default setup.

RecyclableMemoryStream docs state:

Large Pool - Holds large buffers, which are only used when you must have a single, contiguous buffer, such as when you plan to call GetBuffer().

So, if no-one will invoke stream.(Try)GetBuffer, all streams are puzzled together from 128 kbyte, small buffers, and the large pools remain untouched.

Note the reasoning behind that buffer size selection:

RecyclableMemoryStream improves GC performance by ensuring that the larger buffers used for the streams are put into the gen 2 heap and stay there forever. This should cause full collections to happen less frequently. If you pick buffer sizes above 85,000 bytes, then you will ensure these are placed on the large object heap, which is touched even less frequently by the garbage collector.

We need to adapt this size in ChunkedMemoryStream!

@JimBobSquarePants
Copy link
Member Author

We need to adapt this size in ChunkedMemoryStream!

Yep!

@JimBobSquarePants JimBobSquarePants merged commit fca68d1 into master Aug 25, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/stream-pooling branch August 25, 2020 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants