[Piglit] [PATCH 2/2] shader_runner optimization

[Tom Gall]

On Mon, Jan 21, 2013 at 12:49 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 17 January 2013 15:08, Tom Gall <tom.gall at linaro.org> wrote:
>>
>> When shader_runner parses the shader_test data file, add an
>> optimization such that after the [required] section is left to
>> no longer parse the rest of the file.
>>
>> Signed-off-by: Tom Gall <tom.gall at linaro.org>
<snip>
> I'm uncomfortable with this patch for a few reasons:
>
> 1. This patch is intended as a performance optimization, but we have no
> evidence that there's a performance bottleneck here.  In fact, I think it's
> extremely unlikely that shader_runner's parse_required_versions() function
> will ever constitute a performance bottleneck, because the amount of work
> being done in this function to skip the rest of the test file is miniscule
> compared to the work performed by the GL implementation in running the test.
> I don't think we should be making performance improvements in code that
> doesn't need it unless there's some other reason we need the change (e.g.
> readability, maintainability, portability, or paving the way for future
> changes)--it just risks bugs.

Let me potentially convince you otherwise.

> 2. This patch actually introduces a change in behaviour.  Previously, if a
> test had multiple "[require]" sections, parse_required_versions() would
> examine them all.  Granted, I'm not aware of any tests that have multiple
> "[require]" sections (nor any reason to write one), but since it's not
> prohibited, there's a decent chance someone will make one by accident one of
> these days (e.g. by accidentally duplicating the "[require]" line), and if
> they do, this patch will cause them to get confusing errors from
> shader_runner.

If you have multiple [require] sections, I submit that is at worst a
bug and at best asking for trouble.

The last place where one sets a value such as GLSL or GL is what will
win if one sets the same value more than once. What practical use is
that? When one is debugging, you'll likely open the data file, see the
[require] section and stop right there. There's no reason for the
average tourist to think to themselves they should scroll down the
file and look for another require section.

Further, why would one want to specify say GL in one require section
and then have another require section to specify say GLSL further down
the file? This is just being obscure for obscurities sake.

Last if one rewrote the tool a bit, I could see where one might have
more than more than one test across different APIs jammed together,
but again, what practical purpose would that have?
Ex:
[require]
shaders..
test
[require]
shaders
another test
...


Allowing this behavior I think just opens the door to some ugly
testcases which I can't see passing review.

--
Regards,
Tom

"Where's the kaboom!? There was supposed to be an earth-shattering
kaboom!" Marvin Martian
Tech Lead, Graphics Working Group | Linaro.org │ Open source software
for ARM SoCs
w) tom.gall att linaro.org
h) tom_gall att mac.com