[Pixman] [PATCH 1/4] pixman-fast-path: Add over_n_8888 fast path (disabled)

[Oded Gabbay]

On Mon, Aug 24, 2015 at 2:09 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Sat, 22 Aug 2015 13:14:08 -0700
> Matt Turner <mattst88 at gmail.com> wrote:
>
>> On Thu, Aug 20, 2015 at 6:58 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > From: Ben Avison <bavison at riscosopen.org>
>> >
>> > This is a C fast path, useful for reference or for platforms that don't
>> > have their own fast path for this operation.
>> >
>> > This new fast path is initially disabled by putting the entries in the
>> > lookup table after the sentinel. The compiler cannot tell the new code
>> > is not used, so it cannot eliminate the code. Also the lookup table size
>> > will include the new fast path. When the follow-up patch then enables
>> > the new fast path, the binary layout (alignments, size, etc.) will stay
>> > the same compared to the disabled case.
>> >
>> > Keeping the binary layout identical is important for benchmarking on
>> > Raspberry Pi 1. The addresses at which functions are loaded will have a
>> > significant impact on benchmark results, causing unexpected performance
>> > changes. Keeping all function addresses the same across the patch
>> > enabling a new fast path improves the reliability of benchmarks.
>> >
>> > Benchmark results are included in the patch enabling this fast path.
>> >
>> > [Pekka: disabled the fast path, commit message]
>> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>
>> I don't care strongly, but I might just squash 1+2, 3+4 together and
>> make a mention in the commit message of exactly what the benchmark
>> numbers are comparing.
>
> Hi Matt,
>
> yes, that's always a possibility, but then I need to explain what the
> benchmarked code revisions looked like which is prone to
> misunderstanding. So personally I would prefer to land exactly the
> revisions of the code that were tested/benchmarked, so that posterity
> can (at least in theory) repeat the benchmarks if needed. Or has that
> little value?
>
> Maybe I should elaborate on how to iterate benchmarks in the "enable"
> patches.
>
> To me this has documentary value, also in educating others on the
> quirks of rpi1. I don't generally intend to use this style for anything
> but rpi1 specific patches.
>
> Does keeping the patches split make reviewing harder?
>
> I could also keep these 4 as is and squash the future patches?
>
>
> Thanks,
> pq
>
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
>

Hi,
Although I would generally agree with Matt, I believe that due to
rpi-1 quirks, Pekka's approach is correct in this specific case.
I think that reproducing the exact results/benchmarks has more than
enough value to justify the separate patches this time.

Anyway, that's just my 2 cents.

      Oded