[Piglit] [PATCH 2/2] shader_runner optimization

[Paul Berry]

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>
> ---
>  tests/shaders/shader_runner.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 8dfeb2a..6033bda 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -869,7 +869,10 @@ parse_required_versions(struct
> requirement_parse_results *results,
>
>         while (line[0] != '\0') {
>                 if (line[0] == '[')
> -                       in_requirement_section = false;
> +                       if (in_requirement_section)
> +                               break;
> +                       else
> +                               in_requirement_section = false;
>
>                 if (!in_requirement_section) {
>                         if (string_match("[require]", line)) {
> --
> 1.7.10.4
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130121/6e2e21da/attachment.html>