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.

Sign up for GitHub

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

Hoist some of the calculations from loops of 3DMoments() #1818

Merged
merged 4 commits into from
Nov 13, 2021
Merged

Hoist some of the calculations from loops of 3DMoments() #1818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6885d97
hoist some calculations out
kunalspathak Nov 10, 2021
c0d5dfb
Better variable names
kunalspathak Nov 11, 2021
57357b0
Optimize Mark
kunalspathak Nov 12, 2021
0db87a5
Merge branch 'master' into 3DMoments
brianpopow Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions src/ImageSharp/Processing/Processors/Quantization/WuQuantizer{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,19 +417,40 @@ private void Get3DMoments(MemoryAllocator allocator)

for (int r = 1; r < IndexCount; r++)
{
// Currently, RyuJIT hoists the invariants of multi-level nested loop only to the
// immediate outer loop. See https://github.com/dotnet/runtime/issues/61420
// To ensure the calculation doesn't happen repeatedly, hoist some of the calculations
// in the form of ind1* manually.
int ind1R = (r << ((IndexBits * 2) + IndexAlphaBits)) +
(r << (IndexBits + IndexAlphaBits + 1)) +
(r << (IndexBits * 2)) +
(r << (IndexBits + 1)) +
r;

volumeSpan.Clear();

for (int g = 1; g < IndexCount; g++)
{
int ind1G = ind1R +
(g << (IndexBits + IndexAlphaBits)) +
(g << IndexBits) +
g;
int r_g = r + g;

areaSpan.Clear();

for (int b = 1; b < IndexCount; b++)
{
int ind1B = ind1G +
((r_g + b) << IndexAlphaBits) +
b;

Moment line = default;

for (int a = 1; a < IndexAlphaCount; a++)
{
int ind1 = GetPaletteIndex(r, g, b, a);
int ind1 = ind1B + a;

line += momentSpan[ind1];

areaSpan[a] += line;
Expand Down Expand Up @@ -628,13 +649,35 @@ private void Mark(ref Box cube, byte label)

for (int r = cube.RMin + 1; r <= cube.RMax; r++)
{
// Currently, RyuJIT hoists the invariants of multi-level nested loop only to the
// immediate outer loop. See https://github.com/dotnet/runtime/issues/61420
// To ensure the calculation doesn't happen repeatedly, hoist some of the calculations
// in the form of ind1* manually.
int ind1R = (r << ((IndexBits * 2) + IndexAlphaBits)) +
(r << (IndexBits + IndexAlphaBits + 1)) +
(r << (IndexBits * 2)) +
(r << (IndexBits + 1)) +
r;

for (int g = cube.GMin + 1; g <= cube.GMax; g++)
{
int ind1G = ind1R +
(g << (IndexBits + IndexAlphaBits)) +
(g << IndexBits) +
g;
int r_g = r + g;

for (int b = cube.BMin + 1; b <= cube.BMax; b++)
{
int ind1B = ind1G +
((r_g + b) << IndexAlphaBits) +
b;

for (int a = cube.AMin + 1; a <= cube.AMax; a++)
{
tagSpan[GetPaletteIndex(r, g, b, a)] = label;
int index = ind1B + a;

tagSpan[index] = label;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/ImageSharp.Benchmarks/Codecs/EncodeIndexedPng.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public void PngCoreWu()
public void PngCoreWuNoDither()
{
using var memoryStream = new MemoryStream();
var options = new PngEncoder { Quantizer = new WuQuantizer(new QuantizerOptions { Dither = null }) };
var options = new PngEncoder { Quantizer = new WuQuantizer(new QuantizerOptions { Dither = null }), ColorType = PngColorType.Palette };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change so I can get perf coverage for it. Should I keep it or revert it?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. That's an oversight from us.

Very happy to see what other optimizations you can bring as long as they are well commented. Thanks! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, happy to contribute. I think it will be more productive for us to contribute or investigate the performance issues if these benchmarks are in our system. Any progress on #1795?

Copy link
Member

Choose a reason for hiding this comment

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

Leaving that one for @antonfirsov Likely after we complete #1730 and release V2.

Copy link
Member

Choose a reason for hiding this comment

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

Answered:
#1795 (reply in thread)

this.bmpCore.SaveAsPng(memoryStream, options);
}
}
Expand Down