OilPaintingProcessor memory usage improvements #1068

Sergio0694 · 2020-01-06T02:13:39Z

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 👮.
I have provided test coverage for my change (where applicable)

Description

The OilPaintingProcessor was allocating 4 arrays for each single pixel being processed. I've switched to a single, shared float[] array rented from the used configuration, and I'm using that as temporary data for all the 4 processing bins. In doing so, I've also switched to Unsafe.Add for all the read/write operations to the 4 bins, for that extra micro-optimization breeze 🙌

Just noticed those 4 throwaway arrays while working on my other pull request and I couldn't resist 🤣

Sergio0694 · 2020-01-06T03:09:54Z

@JimBobSquarePants Don't merge this one just yet even if it passes the tests, I have another minor optimization done in another commit but I'll wait before pushing that one so that you'll have the CI free for your work 👍🏻
Will probably push it in 12 hours or so, so that it'd be night over there anyway.

codecov · 2020-01-06T13:52:02Z

Codecov Report

Merging #1068 into master will decrease coverage by 8.36%.
The diff coverage is 88.9%.

@@            Coverage Diff             @@
##           master    #1068      +/-   ##
==========================================
- Coverage   89.83%   81.46%   -8.37%     
==========================================
  Files        1200      704     -496     
  Lines       53010    29342   -23668     
  Branches     3847     3290     -557     
==========================================
- Hits        47621    23904   -23717     
- Misses       4561     4745     +184     
+ Partials      828      693     -135

Flag	Coverage Δ
#unittests	`81.46% <88.9%> (?)`

Impacted Files	Coverage Δ
src/ImageSharp/Memory/BufferArea{T}.cs	`97.05% <ø> (ø)`	⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs	`82.66% <ø> (-17.34%)`	⬇️
...cessing/Processors/Convolution/PrewittProcessor.cs	`100% <ø> (ø)`	⬆️
...g/Processors/Filters/LomographProcessor{TPixel}.cs	`100% <ø> (ø)`	⬆️
...cessing/Processors/Convolution/BoxBlurProcessor.cs	`100% <ø> (ø)`	⬆️
...ssors/Binarization/BinaryOrderedDitherProcessor.cs	`90.9% <ø> (-9.1%)`	⬇️
src/ImageSharp/Common/Helpers/ImageMaths.cs	`82.5% <ø> (ø)`	⬆️
...ng/Processors/Convolution/GaussianBlurProcessor.cs	`84.61% <ø> (ø)`	⬆️
...ing/Extensions/Filters/ColorBlindnessExtensions.cs	`100% <ø> (ø)`	⬆️
...c/ImageSharp/Formats/Png/Zlib/ZlibDeflateStream.cs	`68.88% <ø> (+2.22%)`	⬆️
... and 918 more

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 be67977...c89e0b1. Read the comment docs.

Sergio0694 · 2020-01-06T16:05:58Z

@JimBobSquarePants I've pushed my other commit now, though all the tests are failing in the CI for some reasons, I suspect it's just the CI itself that's acting up as before.
The tests for the OilPaintingEffect all pass successfully on my end.
Just an FYI, this PR isn't a priority since it's just a couple tweaks anyway 😊

cc. @antonfirsov in case you want to review the changes in this PR, as you're the optimization guru 😄
Also I was wondering whether it'd be better to add some more comments in the "weirdest" parts, for future reference? I mean eg. in parts like this. It's by no means no SSE/AVX magic, but still 🤔

JimBobSquarePants · 2020-01-07T05:19:44Z

@Sergio0694 I don't think it's all CI issues. Maybe try running your tests locally in 32bit mode.

https://ci.appveyor.com/project/six-labors/imagesharp/builds/29921783

Sergio0694 · 2020-01-07T13:03:11Z

@JimBobSquarePants Ok, this is weird - I've cloned ImageSharp from scratch into a new folder, initialized the submodules and everything, just opened master in Visual Studio and ran the OilPaintTest class, and I still got the same exact errors:

Same issue in the branch for this PR in my for - I might be missing something but I don't think those errors are caused by changes I made in my branch, since master is failing for me as well with the same errors in the official repo.

What's more, the error seems to be with the checks in the RowInterval constructor that's invoked by the ParallelHelper.IterateRows method - it's throwing an ArgumentOutOfRangeException 🤔

brianpopow · 2020-01-07T18:44:18Z

@JimBobSquarePants Ok, this is weird - I've cloned ImageSharp from scratch into a new folder, initialized the submodules and everything, just opened master in Visual Studio and ran the OilPaintTest class, and I still got the same exact errors:

Same issue in the branch for this PR in my for - I might be missing something but I don't think those errors are caused by changes I made in my branch, since master is failing for me as well with the same errors in the official repo.

What's more, the error seems to be with the checks in the RowInterval constructor that's invoked by the ParallelHelper.IterateRows method - it's throwing an ArgumentOutOfRangeException 🤔

Hi Sergio,

i have tried to reproduce your issue, but on the current master all tests are working fine. Also in the current master, i only see 4 oil paint tests:

Sergio0694 · 2020-01-07T20:05:15Z

@brianpopow Mmh... I had literally just cloned the entire repo into a new folder before running those tests, and I'm seeing the same results in my fork too. I have to say I'm very confused at this point 🤔
I might try this again on another PC entirely too at this point, just to be extra safe.

Sergio0694 · 2020-01-09T15:10:14Z

@JimBobSquarePants I'm not really sure about what's going on here - I've merged the latest changes from master and tried to run the tests locally:

Now I only see those 4 that @brianpopow mentioned, and they pass just fine for me here.
I might try to clone the repo again completely from scratch and just copy/paste the changes I made in the OilPaintProcessor there, but I really wonder what's happening in this PR 🤔

Sergio0694 · 2020-01-26T12:41:27Z

So, I've merged all the changes from master, and while I was at it I also managed to throw in some more memory optimizations (in particular I've reduced the number of times the temporary buffer is rented from numRows*columns in each parallel task, to just 1. 🚀

But, I can't seem to be able to figure out what's wrong with the tests. I've tried to clone ImageSharp from scratch, run the OilPainTest class, and again I got this.

The fact that master does build fine in the CI makes me think that somehow that could be caused by something wrong on my end, but then again this PR fails to build on the CI as well 🤷‍♂

I should add though that at least the "full image" tests run fine in this PR, it's just some of the "in box" ones that file. Just like with the ones in master, when I run them locally.

I have to say I'm a bit confused, I wouldn't mind someone taking a look here 😅

Sergio0694 · 2020-01-26T16:31:34Z

@brianpopow Ah, I forgot they had already given the green light to C# 8 features in ImageSharp, otherwise I would've used the new using statements from the start 😄

Why did you revert 692c35b though? 🤔

brianpopow · 2020-01-26T16:35:04Z

@Sergio0694 hi Sergio. I was able to reproduce the issue with your current branch. The error was introduced with the commit 692c35b (I hope its ok that i reverted it). It seems that the memory was corrupt. This needs to be reviewed again, i could not spot the exact error, but im sure this is how it was introduced.

Sergio0694 · 2020-01-26T16:37:22Z

Hey @brianpopow - ah, sure, if that commit was the culprit then that's perfectly ok! 👍
I'll see if I can reintroduce that change though, since that was a pretty decent optimization. Also, I really wonder what's going on on my end at this point, since those tests are failing on my machine even on master, and that obviously can't be right.

Anyway, thank you so much for taking the time to look into this, I appreciate it! 😊

antonfirsov · 2020-01-26T19:43:29Z

@Sergio0694 if I revert @brianpopow's revert commit, the memory corruption is even worse on my PC:

dotnet test -c Debug -f netcoreapp3.1 --filter FullyQualifiedName~OilPaintTest

[xUnit.net 00:00:03.48]     InBox<Rgba32>(provider: Bmp/Car.bmp[Rgba32], levels: 15, brushSize: 10) [FAIL]
The active test run was aborted. Reason: Test host process crashed : Fatal error. Fatal error. Fatal error. Fatal error. Fatal error. Fatal error. Fatal error. Fatal error. Internal CLR error. (0x80131506)
Internal CLR error. (0x80131506)
Internal CLR error. (0x80131506)
Internal CLR error. (0x80131506)
Internal CLR error. (0x80131506)
   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
   at System.IDisposable.Dispose()
   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
Internal CLR error. (0x80131506)
   at SixLabors.ImageSharp.Processing.Processors.Effects.OilPaintingProcessor`1+<>c__DisplayClass2_1[[SixLabors.ImageSharp.PixelFormats.Rgba32, SixLabors.ImageSharp, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null]].<OnFrameApply>b__0(SixLabors.ImageSharp.Memory.RowInterval, System.Memory`1<System.Numerics.Vector4>)
   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
   at SixLabors.ImageSharp.Processing.Processors.Effects.OilPaintingProcessor`1+<>c__DisplayClass2_1[[SixLabors.ImageSharp.PixelFormats.Rgba32, SixLabors.ImageSharp, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null]].<OnFrameApply>b__0(SixLabors.ImageSharp.Memory.RowInterval, System.Memory`1<System.Numerics.Vector4>)
   at SixLabors.ImageSharp.Processing.Processors.Effects.OilPaintingProcessor`1+<>c__DisplayClass2_1[[SixLabors.ImageSharp.PixelFormats.Rgba32, SixLabors.ImageSharp, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null]].<OnFrameApply>b__0(SixLabors.ImageSharp.Memory.RowInterval, System.Memory`1<System.Numerics.Vector4>)
   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)
Internal CLR error. (0x80131506)
   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)
   at SixLabors.ImageSharp.Processing.Processors.Effects.OilPaintingProcessor`1+<>c__DisplayClass2_1[[SixLabors.ImageSharp.PixelFormats.Rgba32, SixLabors.ImageSharp, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null]].<OnFrameApply>b__0(SixLabors.ImageSharp.Memory.RowInterval, System.Memory`1<System.Numerics.Vector4>)
   at SixLabors.ImageSharp.Processing.Processors.Effects.OilPaintingProcessor`1+<>c__DisplayClass2_1[[SixLabors.ImageSharp.PixelFormats.Rgba32, SixLabors.ImageSharp, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null]].<OnFrameApply>b__0(SixLabors.ImageSharp.Memory.RowInterval, System.Memory`1<System.Numerics.Vector4>)
   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)
   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)
   at System.Threading.Tasks.Parallel+<>c__DisplayClass19_0`1[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].<ForWorker>b__1(System.Threading.Tasks.RangeWorker ByRef, Int32, Boolean ByRef)
   at System.Threading.Tasks.Parallel+<>c__DisplayClass19_0`1[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].<ForWorker>b__1(System.Threading.Tasks.RangeWorker ByRef, Int32, Boolean ByRef)   at System.Threading.Tasks.TaskReplicator+Replica`1[[System.Threading.Tasks.RangeWorker, System.Threading.Tasks.Parallel, Version=4.0.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].ExecuteAction(Boolean ByRef)   at System.Threading.Tasks.Parallel+<>c__DisplayClass19_0`1[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].<ForWorker>b__1(System.Threading.Tasks.RangeWorker ByRef, Int32, Boolean ByRef)
   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)   at SixLabors.ImageSharp.Advanced.ParallelUtils.ParallelHelper+<>c__DisplayClass2_0`1[[System.Numerics.Vector4, System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].<IterateRowsWithTempBuffer>b__0(Int32)   at System.Buffers.IMemoryOwner`1[[System.Single, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Memory()
   at System.Threading.Tasks.Parallel+<>c__DisplayClass19_0`1[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].<ForWorker>b__1(System.Threading.Tasks.RangeWorker ByRef, Int32, Boolean ByRef)   at System.Threading.Tasks.TaskReplicator+Replica`1[[System.Threading.Tasks.RangeWorker, System.Threading.Tasks.Parallel, Version=4.0.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].ExecuteAction(Boolean ByRef)
Internal CLR error. (0x80131506)

I wonder how should we proceed. Maybe merge it as-is for now, and try adding furter fixes later?

Sergio0694 · 2020-01-26T20:26:20Z

@antonfirsov Yup, that's what I was thinking about, I'll give that a try!
It's weird though that there's quite a lot of unsafe indexing already, but all the tests are passing just fine right now. I wonder what's going on there. I have a few hours of free time, I'll let you know as soon as I have an update on this! 👍

Also, I think it'd be best to wait until all the issues have been fixed, I'm a bit worried that there might still be some incorrect indexing going on that's just not being picked up by the tests by chance.
Merging that would be kinda bad 😆

EDIT: tried the Span<T> troubleshoot, no errors. The current branch is safe.

EDIT 2: I think I've found the issue with that commit 👍

EDIT 3: yes, I failed again, but this time I know what's going on 🙈

EDIT 4: victory at last! 🎉

Sergio0694 · 2020-01-26T21:26:55Z

@antonfirsov Managed to fix all the issues and added some more minor optimizations, I'm pretty happy about the implementation right now. I've added you as a reviewer, let me know what you think! 😊

@JimBobSquarePants As promised, the PR is ready to merge now! 🚀

antonfirsov

I'm not familiar with the underlying algorithm, so I have limited understanding here, but the refactor seems OK, and I trust the tests enough to assume that the changes are correct.

Sergio0694 · 2020-01-26T21:55:49Z

@antonfirsov I'll be honest, I really have no clue about the details of the underlying algorithms either, I've just focused on doing some general memory optimizations, trusting the tests as well 😅

antonfirsov · 2020-01-26T22:05:26Z

@Sergio0694 been there so many times with jpeg!

JimBobSquarePants

Very nice, just some minor stylistic issues.

src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs

JimBobSquarePants

LGTM. Very much appreciated! 👍

Sergio0694 · 2020-01-27T01:07:39Z

Yeah! The RC1 is one tiny step closer! 🚀🍻

Sergio0694 added 2 commits January 6, 2020 03:09

Added memory pooling in the OilPaintingProcessor

c21cb57

Switched to vectorized pixel conversions

692c35b

JimBobSquarePants added the area:performance label Jan 6, 2020

JimBobSquarePants added this to the 1.0.0-rc1 milestone Jan 6, 2020

Merge branch 'master' into improvement/oil-processor-memory-usage

be67977

Sergio0694 added 4 commits January 26, 2020 13:01

Merge branch 'master' into improvement/oil-processor-memory-usage

9d97a45

Removed incorrect using directive

0e61f0e

Reduced number of memory allocations

71cf6e8

Added missing using statement

17ff3e1

brianpopow added 2 commits January 26, 2020 17:19

Revert changes from 692c35b: "Switched to vectorized pixel conversions"

6d7c83a

Use using declarations to reduce nesting

5e3a190

Sergio0694 added 4 commits January 26, 2020 21:38

Fixed a rollback error

c2fe3dc

Reintroduced bulk ToVector4 source conversion

054750a

Fixed indexing of source row span

c5de8ce

Reimplemented bulk conversion for target row

e9ed9e8

Sergio0694 requested a review from antonfirsov January 26, 2020 21:26

antonfirsov approved these changes Jan 26, 2020

View reviewed changes

JimBobSquarePants requested changes Jan 27, 2020

View reviewed changes

src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs Outdated Show resolved Hide resolved

src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs Show resolved Hide resolved

Removed using statements, added detailed comment

c89e0b1

JimBobSquarePants approved these changes Jan 27, 2020

View reviewed changes

JimBobSquarePants merged commit 7859e71 into SixLabors:master Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OilPaintingProcessor memory usage improvements #1068

OilPaintingProcessor memory usage improvements #1068

Sergio0694 commented Jan 6, 2020 •

edited

Loading

Sergio0694 commented Jan 6, 2020

codecov bot commented Jan 6, 2020 •

edited

Loading

Sergio0694 commented Jan 6, 2020

JimBobSquarePants commented Jan 7, 2020

Sergio0694 commented Jan 7, 2020 •

edited

Loading

brianpopow commented Jan 7, 2020

Sergio0694 commented Jan 7, 2020

Sergio0694 commented Jan 9, 2020

Sergio0694 commented Jan 26, 2020

Sergio0694 commented Jan 26, 2020

brianpopow commented Jan 26, 2020

Sergio0694 commented Jan 26, 2020

antonfirsov commented Jan 26, 2020 •

edited

Loading

Sergio0694 commented Jan 26, 2020 •

edited

Loading

Sergio0694 commented Jan 26, 2020 •

edited

Loading

antonfirsov left a comment •

edited

Loading

Sergio0694 commented Jan 26, 2020

antonfirsov commented Jan 26, 2020

JimBobSquarePants left a comment

JimBobSquarePants left a comment

Sergio0694 commented Jan 27, 2020

OilPaintingProcessor memory usage improvements #1068

OilPaintingProcessor memory usage improvements #1068

Conversation

Sergio0694 commented Jan 6, 2020 • edited Loading

Prerequisites

Description

Sergio0694 commented Jan 6, 2020

codecov bot commented Jan 6, 2020 • edited Loading

Codecov Report

Sergio0694 commented Jan 6, 2020

JimBobSquarePants commented Jan 7, 2020

Sergio0694 commented Jan 7, 2020 • edited Loading

brianpopow commented Jan 7, 2020

Sergio0694 commented Jan 7, 2020

Sergio0694 commented Jan 9, 2020

Sergio0694 commented Jan 26, 2020

Sergio0694 commented Jan 26, 2020

brianpopow commented Jan 26, 2020

Sergio0694 commented Jan 26, 2020

antonfirsov commented Jan 26, 2020 • edited Loading

Sergio0694 commented Jan 26, 2020 • edited Loading

Sergio0694 commented Jan 26, 2020 • edited Loading

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

Sergio0694 commented Jan 26, 2020

antonfirsov commented Jan 26, 2020

JimBobSquarePants left a comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Sergio0694 commented Jan 27, 2020

Sergio0694 commented Jan 6, 2020 •

edited

Loading

codecov bot commented Jan 6, 2020 •

edited

Loading

Sergio0694 commented Jan 7, 2020 •

edited

Loading

antonfirsov commented Jan 26, 2020 •

edited

Loading

Sergio0694 commented Jan 26, 2020 •

edited

Loading

Sergio0694 commented Jan 26, 2020 •

edited

Loading

antonfirsov left a comment •

edited

Loading