-
Notifications
You must be signed in to change notification settings - Fork 45
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #82 - improve performance of OptimizingMediaCache #83
fix #82 - improve performance of OptimizingMediaCache #83
Conversation
finally | ||
{ | ||
// release resources used by the optimization task | ||
originalMediaStream?.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test that in the case where the optimisation failed for some reason the original stream is returned and original image is rendered? As a test you could use the corrupted image from here #68
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that already done in ShouldNotSquishCorruptedJpegLossy in JpegOptimOptimizerTests? If the OptimizingMediaCache.cs needs to be tested we need to add fakes for the Sitecore objects as well.
If still needed I'll try to have a look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point. Would be nice to have tests for OptimizingMediaCache if you are volunteering :). I'll give this PR a test soon.
@@ -84,4 +84,7 @@ | |||
<None Remove="Default Config Files\Dianoga.WebP.CDN.config.disabled" /> | |||
<None Remove="Dianoga Tools\libwebp\COPYING.txt" /> | |||
</ItemGroup> | |||
<ItemGroup> | |||
<PackageReference Include="System.Threading.Tasks.Dataflow" Version="5.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looking at this https://www.nuget.org/packages/System.Threading.Tasks.Dataflow/5.0.0 and this https://stackoverflow.com/questions/23245713/is-tpl-dataflow-included-with-either-net-4-5-or-net-4-5-1/23245803 looks like we will need to add this dependency that will flow up. Seems like this will be fine on .NET 4.5+ but I would love it to be tested on Sitecore 8 before I push this out, so will tag this as pre-release when I push the package.
Found a concurrency issue testing locally with MaxConcurrentThreads = -1
With the value set to 1 everything works as expected. However with the value set to 4 I get the same errors. This is on a 6 core CPU. So should we hardcode the setting to 1 and not allow it to be changed? |
Set MaxConcurrentThreads < 1 to disable limiting concurrent optimization tasks | ||
--> | ||
<processor type="Dianoga.Invokers.MediaCacheAsync.Pipelines.Initialize.MediaCacheReplacer, Dianoga"> | ||
<MaxConcurrentThreads>-1</MaxConcurrentThreads> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See separate comment about a concurrency issue
this change allows limiting the maximum concurrent optimization tasks and reduces the overall memory footprint