Prototype For Fast Bulk Sampling Of Pixels

[danieldresser-ie]

I was doing some experimenting with some of our more expensive image operations, and found that Sampler.sample() is a big bottleneck. In cases where we need to sample every pixel in a region, we can do much better by doing the iteration within Sampler, where we can iterate through every pixel in the region within a particular tile with a single call to cachedData.

I was a bit unsure how to demonstrate the benefit from this ... Resample with a non-separable filter should benefit because it needs to do an enormous amount of sampling ... but the only case we have currently of a non-separable filter is a filter which is incredibly slow to evaluate, so it doesn't really demonstrate the benefit.

I've ended up demonstrating this on RankFilter - this is a little silly, because RankFilter doesn't need to do all this repeated sampling of the same pixels, but for our current naive implementation, it does show a noticeable speedup, 4X for Erode/Dilate, and 2X for Median.

A more practical demonstration of benefit would be optimizing Blur, but that feels a little bit more contentious, because doing that efficiently would probably require adding a special case to Resample for when the matrix is an identity and the filter weights are identical for every pixel ... I think this makes a lot of sense, but perhaps should be a separate PR.

I think this is looking pretty good, though I wish I had slightly more thorough tests for the rank filters in order to validate this sort of work, and ideally I would double check that visitPixels is fully getting inlined and optimized away properly - the goal is that copying a scanline into a vector should be optimized to basically a memcpy per tile.

[johnhaddon]

Thanks Daniel, I like it! Few rushed comments inline - trying to get it posted before our meeting which started 2 minutes ago :)

[johnhaddon]

Would you have any objection to F( const V2i &p, float value )?

[danieldresser-ie]

I guess there's enough similarity to the previous version of this that there's no need for a separate PR, even though just about everything has been rewritten.

visitPixels does now guarantee a scanline order. Bugs are fixed, and it's being used more broadly ( along with a few other miscellany perf improvements, like "bilinear" mode for Warp, which just calls sampler.sample() ... allowing us to test the perf of that, and giving a faster option for Warp when we're not picky about quality ).

Trying to look at all the performance impacts this has is a lot of data ... I threw together some quick and dirty scripts in the final commit to help produce a graph ... it's not pretty, and is a little untested ( ImageEngine doesn't currently have matplotlib installed, so I just used some random online site to execute the last part ), but it ends up with a graph something like this, for the last 11 commits of this PR:

I collected a whole bunch of data, and basically kept anything that is plausibly interesting ... including a couple of things that I think are just noise, but I didn't want to cherry-pick and risk confirmation bias. I think this pretty clearly indicates the benefit of all these changes.

The one actual regression is a 15% performance loss in testSimplestUpscalePerf, which uses a box filter to upscale so that every output pixel visits a region that is just a single pixel in the input ( meaning you get the overhead of setting up visitPixels, but don't get the benefit of visiting more than one pixel ). This is a pretty rare case, and probably not that concerning.

[johnhaddon]

Nice one! I've made a few very minor comments inline, but this is looking great, and the results speak for themselves.

Note to self : you will need to update Changes.md after merging.

[danieldresser-ie]

OK, I think that's all feedback addressed. I've squashed it in, because it's a lot of small changes to different commits. The only new commit is: "ImageTransformTest : Pre-cache input for performance tests". I also reran the perf tests just in case, and made a graph that is no less ugly, but slightly more informative:

[johnhaddon]

Thanks Daniel! While updating Changes.md, I tested with a large-radius Blur and was very pleased to see that it was nearly 6x faster, and feels waay speedier in interactive use. Nice one!