I think you've got the right change.

It's strange that removing templates increased the code size.  To be honest, I wouldn't have expected much of a change here - the code size comes from the combination of inputs/outputs supported in the "old" opt code path.  That is unchanged here.

I would guess that the apk compression is somehow affected.  Kind of hard to predict that though.

Regardless, this class is large and that is a known issue.  I'm really focused on improving that - this refactor (and RasterPipeline in general) is a step in that direction.

FWIW, we've landed some recent changes that improve the size of this class.  The effort is still ongoing though, so I will leave this bug opened.

Looked into this a bit more by:
1. Compiling with & without the patch in question (reverted it within //third_party/skia)
2. Ran on both copies of libchrome.so:
  nm --size-sort --reverse-sort --demangle --synthetic lib.unstripped/libchrome.so | perl -p -e 's/constprop.*?\]/constprop]/g' > nm.out
3. Diff'ed with:
  diff a/nm.out b/nm.out | grep '^>'
  diff a/nm.out b/nm.out | grep '^<'

I've attached the outputs.

Deleted: nm-diff.txt 55.7 KB

nm-diff.txt 55.7 KB View Download

Some analysis:
- < prefix is with the new CL (> is with it reverted)
- There are 23 fewer symbols
- Sanity checked that the sum of the adds vs sum of removed = 85,076
- sum(color_xform_RGBA) is 88kb larger after vs before

So, my best guess is that this is a case of template explosion.

Matt - I'm not sure how to test performance of these things, but from inspection it looks like only kAlphaType and kCSM are used within the loops of color_xform_RGBA. Would you be able to see if converting template parameters to normal parameters would reduce codesize while maintaining adequate performance?

From looking at the file:
- kDst and kSrc look to not be used inside the loops
- kCSM and kAlphaType are used, but maybe further savings could be had if the loops are pulled into a separate (templated) helper, and the color_xform_RGBA be changed to have no template parameters?
- And maybe ever further savings by converting most other non-loop-containing functions in the file?


Note for future reader: Summing up of hex columns done via:
diff binsize.orig/nm.out binsize/nm.out |grep '^<' | grep color_xform_RGBA | cut -c3-10 | paste -sd+ - | tr 'a-z' 'A-Z' | cat <(echo "ibase=16;") - | bc

About why the alert didn't fire--looks like it actually did fire, but misplaced at r443362. Unfortunately, the perf sheriff marked it as ignored. I think that likely happened because they assumed 86kib was not a big deal. +benhenry for ideas on how to prevent this happening in the future?

Seems like this is maybe fixed by https://skia-review.googlesource.com/c/8848/?

It reduced the binary size by 172100 bytes

Woohoo, yes!

Do you know what that translates to in terms on apk size?

We store the native library uncompressed, so the apk size change is the same.

Ahh thanks, I misread bytes vs. kilobytes - so we've decreased apk by 168 kb.