i965: Enable disk shader cache by default

Submitted by Jordan Justen on Nov. 9, 2017, 12:58 a.m.

Details

Message ID 20171109005806.9944-1-jordan.l.justen@intel.com
State New
Headers show
Series "i965: Enable disk shader cache by default" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Jordan Justen Nov. 9, 2017, 12:58 a.m.
f9d5a7add42af5a2e4410526d1480a08f41317ae along with
a16dc04ad51c32e5c7d136e4dd6273d983385d3f appears to have fixed the one
known regression with shader cache. (Deus Ex instability.)

We should enable the shader cache by default to stabilize it before
the next major Mesa release.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 docs/relnotes/17.4.0.html                  | 2 +-
 src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/docs/relnotes/17.4.0.html b/docs/relnotes/17.4.0.html
index f81b5bd62d3..48dcd5cce38 100644
--- a/docs/relnotes/17.4.0.html
+++ b/docs/relnotes/17.4.0.html
@@ -44,7 +44,7 @@  Note: some of the new features are only available with certain drivers.
 </p>
 
 <ul>
-<li>Disk shader cache support for i965 when MESA_GLSL_CACHE_DISABLE environment variable is set to "0" or "false"</li>
+<li>Disk shader cache support for i965</li>
 </ul>
 
 <h2>Bug fixes</h2>
diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c b/src/mesa/drivers/dri/i965/brw_disk_cache.c
index 853ea98af03..cd0524c5cbf 100644
--- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -420,9 +420,6 @@  void
 brw_disk_cache_init(struct brw_context *brw)
 {
 #ifdef ENABLE_SHADER_CACHE
-   if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", true))
-      return;
-
    char renderer[10];
    MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
                                    brw->screen->deviceID);

Comments

Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>

Mark may want to consider adding some of the once a day type CI runs for 
this. For example running the test suite for two consecutive runs on the 
same build so that the second run uses the shader cache and also a 
second run the uses MESA_GLSL=cache_fb to force testing of the cache 
fallback path.

On 09/11/17 11:58, Jordan Justen wrote:
> f9d5a7add42af5a2e4410526d1480a08f41317ae along with
> a16dc04ad51c32e5c7d136e4dd6273d983385d3f appears to have fixed the one
> known regression with shader cache. (Deus Ex instability.)
> 
> We should enable the shader cache by default to stabilize it before
> the next major Mesa release.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>   docs/relnotes/17.4.0.html                  | 2 +-
>   src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ---
>   2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/docs/relnotes/17.4.0.html b/docs/relnotes/17.4.0.html
> index f81b5bd62d3..48dcd5cce38 100644
> --- a/docs/relnotes/17.4.0.html
> +++ b/docs/relnotes/17.4.0.html
> @@ -44,7 +44,7 @@ Note: some of the new features are only available with certain drivers.
>   </p>
>   
>   <ul>
> -<li>Disk shader cache support for i965 when MESA_GLSL_CACHE_DISABLE environment variable is set to "0" or "false"</li>
> +<li>Disk shader cache support for i965</li>
>   </ul>
>   
>   <h2>Bug fixes</h2>
> diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> index 853ea98af03..cd0524c5cbf 100644
> --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> @@ -420,9 +420,6 @@ void
>   brw_disk_cache_init(struct brw_context *brw)
>   {
>   #ifdef ENABLE_SHADER_CACHE
> -   if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", true))
> -      return;
> -
>      char renderer[10];
>      MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
>                                      brw->screen->deviceID);
>
On 2017-11-08 17:26:47, Timothy Arceri wrote:
> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
> 
> Mark may want to consider adding some of the once a day type CI runs for 
> this. For example running the test suite for two consecutive runs on the 
> same build so that the second run uses the shader cache and also a 
> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
> fallback path.

Yeah. We discussed this previously, but I don't think it's been
implemented yet. My opinion is that it could perhaps be a weekly test.

We also discussed a nir serialization test, similar to our current nir
clone daily test. I don't think this is implemented yet either.

-Jordan

> 
> On 09/11/17 11:58, Jordan Justen wrote:
> > f9d5a7add42af5a2e4410526d1480a08f41317ae along with
> > a16dc04ad51c32e5c7d136e4dd6273d983385d3f appears to have fixed the one
> > known regression with shader cache. (Deus Ex instability.)
> > 
> > We should enable the shader cache by default to stabilize it before
> > the next major Mesa release.
> > 
> > Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> > ---
> >   docs/relnotes/17.4.0.html                  | 2 +-
> >   src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ---
> >   2 files changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/docs/relnotes/17.4.0.html b/docs/relnotes/17.4.0.html
> > index f81b5bd62d3..48dcd5cce38 100644
> > --- a/docs/relnotes/17.4.0.html
> > +++ b/docs/relnotes/17.4.0.html
> > @@ -44,7 +44,7 @@ Note: some of the new features are only available with certain drivers.
> >   </p>
> >   
> >   <ul>
> > -<li>Disk shader cache support for i965 when MESA_GLSL_CACHE_DISABLE environment variable is set to "0" or "false"</li>
> > +<li>Disk shader cache support for i965</li>
> >   </ul>
> >   
> >   <h2>Bug fixes</h2>
> > diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > index 853ea98af03..cd0524c5cbf 100644
> > --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> > @@ -420,9 +420,6 @@ void
> >   brw_disk_cache_init(struct brw_context *brw)
> >   {
> >   #ifdef ENABLE_SHADER_CACHE
> > -   if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", true))
> > -      return;
> > -
> >      char renderer[10];
> >      MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
> >                                      brw->screen->deviceID);
> >
Jordan Justen <jordan.l.justen@intel.com> writes:

> On 2017-11-08 17:26:47, Timothy Arceri wrote:
>> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>> 
>> Mark may want to consider adding some of the once a day type CI runs for 
>> this. For example running the test suite for two consecutive runs on the 
>> same build so that the second run uses the shader cache and also a 
>> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
>> fallback path.
>
> Yeah. We discussed this previously, but I don't think it's been
> implemented yet. My opinion is that it could perhaps be a weekly test.

This automation is implemented now. It found the issues reported in
https://bugs.freedesktop.org/show_bug.cgi?id=103988

My opinion is that this test strategy is inadequate.  Adding a dimension
to the test matrix has high cost, especially when combined with other
dimensions of the test matrix (does shader cache need to be tested for
32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).

Since 103988 is not hardware specific, and is not a regression, it's not
something that could only have been caught by CI.  I'm surprised to find
it at this point, considering piglit was used to validate the feature.

I'd be happy if there was at least a smoke test where complex programs
are cached, with the intent of exercising the shader cache mechanism
directly.  It doesn't have to be exhaustive to be effective.  Seems like
a good idea to at least directly test the issue that was fixed in
103988 tests.

> We also discussed a nir serialization test, similar to our current nir
> clone daily test. I don't think this is implemented yet either.
>
> -Jordan
>
>> 
>> On 09/11/17 11:58, Jordan Justen wrote:
>> > f9d5a7add42af5a2e4410526d1480a08f41317ae along with
>> > a16dc04ad51c32e5c7d136e4dd6273d983385d3f appears to have fixed the one
>> > known regression with shader cache. (Deus Ex instability.)
>> > 
>> > We should enable the shader cache by default to stabilize it before
>> > the next major Mesa release.
>> > 
>> > Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>> > ---
>> >   docs/relnotes/17.4.0.html                  | 2 +-
>> >   src/mesa/drivers/dri/i965/brw_disk_cache.c | 3 ---
>> >   2 files changed, 1 insertion(+), 4 deletions(-)
>> > 
>> > diff --git a/docs/relnotes/17.4.0.html b/docs/relnotes/17.4.0.html
>> > index f81b5bd62d3..48dcd5cce38 100644
>> > --- a/docs/relnotes/17.4.0.html
>> > +++ b/docs/relnotes/17.4.0.html
>> > @@ -44,7 +44,7 @@ Note: some of the new features are only available with certain drivers.
>> >   </p>
>> >   
>> >   <ul>
>> > -<li>Disk shader cache support for i965 when MESA_GLSL_CACHE_DISABLE environment variable is set to "0" or "false"</li>
>> > +<li>Disk shader cache support for i965</li>
>> >   </ul>
>> >   
>> >   <h2>Bug fixes</h2>
>> > diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> > index 853ea98af03..cd0524c5cbf 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> > +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> > @@ -420,9 +420,6 @@ void
>> >   brw_disk_cache_init(struct brw_context *brw)
>> >   {
>> >   #ifdef ENABLE_SHADER_CACHE
>> > -   if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", true))
>> > -      return;
>> > -
>> >      char renderer[10];
>> >      MAYBE_UNUSED int len = snprintf(renderer, sizeof(renderer), "i965_%04x",
>> >                                      brw->screen->deviceID);
>> >
On 2017-12-05 09:13:11, Mark Janes wrote:
> Jordan Justen <jordan.l.justen@intel.com> writes:
> 
> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
> >> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
> >> 
> >> Mark may want to consider adding some of the once a day type CI runs for 
> >> this. For example running the test suite for two consecutive runs on the 
> >> same build so that the second run uses the shader cache and also a 
> >> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
> >> fallback path.
> >
> > Yeah. We discussed this previously, but I don't think it's been
> > implemented yet. My opinion is that it could perhaps be a weekly test.
> 
> This automation is implemented now. It found the issues reported in
> https://bugs.freedesktop.org/show_bug.cgi?id=103988
> 
> My opinion is that this test strategy is inadequate.

Meaning you think we cannot enable i965 shader cache for the 18.0
release?

We are already ~1.5 months away from the next stable branch point. If
we want to enable this in 18.0, we should be using this time to see if
enabling the cache by default has some large unexpected side effect in
our graphics stack, or breaks real-world apps.

> Adding a dimension
> to the test matrix has high cost, especially when combined with other
> dimensions of the test matrix (does shader cache need to be tested for
> 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).

Are you saying this is too high cost to run per check-in? Do you need
to disable it for the health of CI? I think I proposed that daily, or
perhaps even weekly would be adequate.

These tests are already identifying some corner case issues. I'm not
sure these would have impacted real-world apps yet, but I do think it
is a good idea to run these tests regularly.

You say this test strategy is inadequate. Perhaps. I do think it needs
to be part of our test strategy. There is no way we are going to hit
all the corners of the API better than running all of our tests with
the cache enabled. Do you agree?

> Since 103988 is not hardware specific, and is not a regression, it's not
> something that could only have been caught by CI.  I'm surprised to find
> it at this point, considering piglit was used to validate the feature.
> 
> I'd be happy if there was at least a smoke test where complex programs
> are cached, with the intent of exercising the shader cache mechanism
> directly.  It doesn't have to be exhaustive to be effective.  Seems like
> a good idea to at least directly test the issue that was fixed in
> 103988 tests.

It could be interesting to define a MESA extension to control or query
the shader cache. Today, the shader cache is a nearly 'invisible'
feature. There are a few env-vars, but I wouldn't call a public
interface.

The downside of defining an extension is that it might constrain
changes to the shader cache implementation. Or, if the interface is
too complex, then it may be tough for some drivers to implement.

But, what if we get around to defining and implementing this extenion,
(and a few piglit tests that makes use of it), by mid-January? It'll
still be pretty difficult to argue we should enable the shader cache
on i965 for 18.0, since we burned all the time to let it be tested on
master against real-world apps.

Also, while you say our current test strategy is inadequate, I do
think it is still far more comprehensive than anything we will
accomplish by defining a new extension and writing a few, or even a
few hundred tests against the new extension.

-Jordan
Jordan Justen <jordan.l.justen@intel.com> writes:

> On 2017-12-05 09:13:11, Mark Janes wrote:
>> Jordan Justen <jordan.l.justen@intel.com> writes:
>> 
>> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
>> >> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>> >> 
>> >> Mark may want to consider adding some of the once a day type CI runs for 
>> >> this. For example running the test suite for two consecutive runs on the 
>> >> same build so that the second run uses the shader cache and also a 
>> >> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
>> >> fallback path.
>> >
>> > Yeah. We discussed this previously, but I don't think it's been
>> > implemented yet. My opinion is that it could perhaps be a weekly test.
>> 
>> This automation is implemented now. It found the issues reported in
>> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>> 
>> My opinion is that this test strategy is inadequate.
>
> Meaning you think we cannot enable i965 shader cache for the 18.0
> release?

I haven't heard anyone express confidence that this feature is bug-free,
and I don't know on what basis that claim could be made.  I appreciate
that a lot of have manual testing has been done.  Do you feel confident
that the cache can be enabled for all mesa customers without disruption?

> We are already ~1.5 months away from the next stable branch point. If
> we want to enable this in 18.0, we should be using this time to see if
> enabling the cache by default has some large unexpected side effect in
> our graphics stack, or breaks real-world apps.

I agree.  We should encourage as many users as possible to enable the
shader cache in their environments.  At least one stable release should
be out with a working cache, where the feature can be enabled by those
who are eager to benefit from it.  I assume there will be a lot of them,
and they could flush out issues for everyone who has no idea what a
shader cache is.

>> Adding a dimension to the test matrix has high cost, especially when
>> combined with other dimensions of the test matrix (does shader cache
>> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>
> Are you saying this is too high cost to run per check-in? Do you need
> to disable it for the health of CI? I think I proposed that daily, or
> perhaps even weekly would be adequate.

Certainly, the test time per line of shader cache code is massively
higher than any other feature, even if you run it just once a month.
Other features have tests that run in milliseconds, not 30min * 20
machines.

Beyond poor return on execution time, there is the simple fact that
whoever is running the CI needs to manually look at shader cache results
separately from the rest of the tests.  Unit testing is effective
because coverage can be added at approximately zero marginal cost.

3 years from now, will we still be looking at separate weekly shader
cache test runs to make sure it's working?

> These tests are already identifying some corner case issues. I'm not
> sure these would have impacted real-world apps yet, but I do think it
> is a good idea to run these tests regularly.
>
> You say this test strategy is inadequate. Perhaps. I do think it needs
> to be part of our test strategy. There is no way we are going to hit
> all the corners of the API better than running all of our tests with
> the cache enabled. Do you agree?

Tests should strive to cover the implementation, not the API.  Shader
cache is a unique feature, because it affects a large part of the API.
It doesn't necessarily follow that covering the API will cover the
feature, or that that is an effective test strategy.

Knowing nothing of the implementation, I would look to similar projects
(eg: ccache) to see what test strategies they employ.  There are
probably issues they've encountered that we have not thought of.

Another approach would be to write real unit tests for bugs found during
the course of the shader cache implementation.  Those test cases would
help developers know that they broke a cache feature without having to
rely on a weekly CI run.

>> Since 103988 is not hardware specific, and is not a regression, it's not
>> something that could only have been caught by CI.  I'm surprised to find
>> it at this point, considering piglit was used to validate the feature.
>> 
>> I'd be happy if there was at least a smoke test where complex programs
>> are cached, with the intent of exercising the shader cache mechanism
>> directly.  It doesn't have to be exhaustive to be effective.  Seems like
>> a good idea to at least directly test the issue that was fixed in
>> 103988 tests.
>
> It could be interesting to define a MESA extension to control or query
> the shader cache. Today, the shader cache is a nearly 'invisible'
> feature. There are a few env-vars, but I wouldn't call a public
> interface.
>
> The downside of defining an extension is that it might constrain
> changes to the shader cache implementation. Or, if the interface is
> too complex, then it may be tough for some drivers to implement.

Why is an extension necessary?  By comparison, GCC has no interface to
query the statistics for ccache.  A utility that can read the cache
files and report hits/misses would at least inform us that the cache is
functioning.  The utility would be useful in writing real unit tests for
the feature.

> But, what if we get around to defining and implementing this extenion,
> (and a few piglit tests that makes use of it), by mid-January? It'll
> still be pretty difficult to argue we should enable the shader cache
> on i965 for 18.0, since we burned all the time to let it be tested on
> master against real-world apps.

Personally, I think it is already difficult to argue that the cache
should be on by default in 18.0.

> Also, while you say our current test strategy is inadequate, I do
> think it is still far more comprehensive than anything we will
> accomplish by defining a new extension and writing a few, or even a
> few hundred tests against the new extension.

In the short term, we should do whatever is expedient to try to test the
feature as much as we can.

In the longer term, a weekly test is going to be costly and ineffective
at preventing regressions.  Tests need to be runnable for developers to
use them as part of their process.  If a test can only be run at great
expense in a weekly CI, then any fragility in the feature will be
discovered a week after a bug has been pushed to master.

> -Jordan
On 06/12/17 09:49, Mark Janes wrote:
> Jordan Justen <jordan.l.justen@intel.com> writes:
> 
>> On 2017-12-05 09:13:11, Mark Janes wrote:
>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>
>>>> On 2017-11-08 17:26:47, Timothy Arceri wrote:
>>>>> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>>>>>
>>>>> Mark may want to consider adding some of the once a day type CI runs for
>>>>> this. For example running the test suite for two consecutive runs on the
>>>>> same build so that the second run uses the shader cache and also a
>>>>> second run the uses MESA_GLSL=cache_fb to force testing of the cache
>>>>> fallback path.
>>>>
>>>> Yeah. We discussed this previously, but I don't think it's been
>>>> implemented yet. My opinion is that it could perhaps be a weekly test.
>>>
>>> This automation is implemented now. It found the issues reported in
>>> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>>>
>>> My opinion is that this test strategy is inadequate.
>>
>> Meaning you think we cannot enable i965 shader cache for the 18.0
>> release?
> 
> I haven't heard anyone express confidence that this feature is bug-free,
> and I don't know on what basis that claim could be made.  I appreciate
> that a lot of have manual testing has been done.  Do you feel confident
> that the cache can be enabled for all mesa customers without disruption?
> 

I would suggest enabling it now in master it can be switched off again 
before release. For what its worth besides 1 or 2 dev branch related 
issues we still haven't had any shader cache related regressions 
reported for radeonsi (or any of the other gallium drivers with shader 
cache) and its been enabled for a couple of releases now.


>> We are already ~1.5 months away from the next stable branch point. If
>> we want to enable this in 18.0, we should be using this time to see if
>> enabling the cache by default has some large unexpected side effect in
>> our graphics stack, or breaks real-world apps.
> 
> I agree.  We should encourage as many users as possible to enable the
> shader cache in their environments.  At least one stable release should
> be out with a working cache, where the feature can be enabled by those
> who are eager to benefit from it.  I assume there will be a lot of them,
> and they could flush out issues for everyone who has no idea what a
> shader cache is.
> 
>>> Adding a dimension to the test matrix has high cost, especially when
>>> combined with other dimensions of the test matrix (does shader cache
>>> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>>
>> Are you saying this is too high cost to run per check-in? Do you need
>> to disable it for the health of CI? I think I proposed that daily, or
>> perhaps even weekly would be adequate.
> 
> Certainly, the test time per line of shader cache code is massively
> higher than any other feature, even if you run it just once a month.
> Other features have tests that run in milliseconds, not 30min * 20
> machines.
> 
> Beyond poor return on execution time, there is the simple fact that
> whoever is running the CI needs to manually look at shader cache results
> separately from the rest of the tests.  Unit testing is effective
> because coverage can be added at approximately zero marginal cost.
> 
> 3 years from now, will we still be looking at separate weekly shader
> cache test runs to make sure it's working?
> 
>> These tests are already identifying some corner case issues. I'm not
>> sure these would have impacted real-world apps yet, but I do think it
>> is a good idea to run these tests regularly.
>>
>> You say this test strategy is inadequate. Perhaps. I do think it needs
>> to be part of our test strategy. There is no way we are going to hit
>> all the corners of the API better than running all of our tests with
>> the cache enabled. Do you agree?
> 
> Tests should strive to cover the implementation, not the API.  Shader
> cache is a unique feature, because it affects a large part of the API.
> It doesn't necessarily follow that covering the API will cover the
> feature, or that that is an effective test strategy.
> 
> Knowing nothing of the implementation, I would look to similar projects
> (eg: ccache) to see what test strategies they employ.  There are
> probably issues they've encountered that we have not thought of.
> 
> Another approach would be to write real unit tests for bugs found during
> the course of the shader cache implementation.  Those test cases would
> help developers know that they broke a cache feature without having to
> rely on a weekly CI run.

The problem is these tests would just be.

1. Compile shaders
2. Run program
3. Recompile all the same shaders from Step 1
4. Re-run the program in the same was as Step 2

Shader-cache is not like other features in that you need to exercise 
pretty much ever other feature (hence re-running piglit) in order to be 
confident you have full coverage.

> 
>>> Since 103988 is not hardware specific, and is not a regression, it's not
>>> something that could only have been caught by CI.  I'm surprised to find
>>> it at this point, considering piglit was used to validate the feature.
>>>
>>> I'd be happy if there was at least a smoke test where complex programs
>>> are cached, with the intent of exercising the shader cache mechanism
>>> directly.  It doesn't have to be exhaustive to be effective.  Seems like
>>> a good idea to at least directly test the issue that was fixed in
>>> 103988 tests.
>>
>> It could be interesting to define a MESA extension to control or query
>> the shader cache. Today, the shader cache is a nearly 'invisible'
>> feature. There are a few env-vars, but I wouldn't call a public
>> interface.
>>
>> The downside of defining an extension is that it might constrain
>> changes to the shader cache implementation. Or, if the interface is
>> too complex, then it may be tough for some drivers to implement.
> 
> Why is an extension necessary?  By comparison, GCC has no interface to
> query the statistics for ccache.  A utility that can read the cache
> files and report hits/misses would at least inform us that the cache is
> functioning.  The utility would be useful in writing real unit tests for
> the feature.

Ignoring the extension. Testing hit/miss only tests a small part of the 
cache, and is already covered by the 'make check' tests. The real test 
is to make sure we cached/restored things correctly.

> 
>> But, what if we get around to defining and implementing this extenion,
>> (and a few piglit tests that makes use of it), by mid-January? It'll
>> still be pretty difficult to argue we should enable the shader cache
>> on i965 for 18.0, since we burned all the time to let it be tested on
>> master against real-world apps.
> 
> Personally, I think it is already difficult to argue that the cache
> should be on by default in 18.0.
> 
>> Also, while you say our current test strategy is inadequate, I do
>> think it is still far more comprehensive than anything we will
>> accomplish by defining a new extension and writing a few, or even a
>> few hundred tests against the new extension.
> 
> In the short term, we should do whatever is expedient to try to test the
> feature as much as we can.
> 
> In the longer term, a weekly test is going to be costly and ineffective
> at preventing regressions.  Tests need to be runnable for developers to
> use them as part of their process.  If a test can only be run at great
> expense in a weekly CI, then any fragility in the feature will be
> discovered a week after a bug has been pushed to master.

Unfortunately I still haven't seen any better suggestions :(

> 
>> -Jordan
On 2017-12-05 14:49:28, Mark Janes wrote:
> Jordan Justen <jordan.l.justen@intel.com> writes:
> 
> > On 2017-12-05 09:13:11, Mark Janes wrote:
> >> Jordan Justen <jordan.l.justen@intel.com> writes:
> >> 
> >> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
> >> >> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
> >> >> 
> >> >> Mark may want to consider adding some of the once a day type CI runs for 
> >> >> this. For example running the test suite for two consecutive runs on the 
> >> >> same build so that the second run uses the shader cache and also a 
> >> >> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
> >> >> fallback path.
> >> >
> >> > Yeah. We discussed this previously, but I don't think it's been
> >> > implemented yet. My opinion is that it could perhaps be a weekly test.
> >> 
> >> This automation is implemented now. It found the issues reported in
> >> https://bugs.freedesktop.org/show_bug.cgi?id=103988
> >> 
> >> My opinion is that this test strategy is inadequate.
> >
> > Meaning you think we cannot enable i965 shader cache for the 18.0
> > release?
> 
> I haven't heard anyone express confidence that this feature is bug-free,
> and I don't know on what basis that claim could be made.  I appreciate
> that a lot of have manual testing has been done.  Do you feel confident
> that the cache can be enabled for all mesa customers without disruption?

If we had enabled it two months ago, then yes, we would have worked
through the niche issues, or discovered the so-far-hidden major land
mine. At this point, it's getting risky. In a month, I will say no.

> > We are already ~1.5 months away from the next stable branch point. If
> > we want to enable this in 18.0, we should be using this time to see if
> > enabling the cache by default has some large unexpected side effect in
> > our graphics stack, or breaks real-world apps.
> 
> I agree.  We should encourage as many users as possible to enable the
> shader cache in their environments.  At least one stable release should
> be out with a working cache, where the feature can be enabled by those
> who are eager to benefit from it.  I assume there will be a lot of them,
> and they could flush out issues for everyone who has no idea what a
> shader cache is.
> 
> >> Adding a dimension to the test matrix has high cost, especially when
> >> combined with other dimensions of the test matrix (does shader cache
> >> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
> >
> > Are you saying this is too high cost to run per check-in? Do you need
> > to disable it for the health of CI? I think I proposed that daily, or
> > perhaps even weekly would be adequate.
> 
> Certainly, the test time per line of shader cache code is massively
> higher than any other feature, even if you run it just once a month.
> Other features have tests that run in milliseconds, not 30min * 20
> machines.

The scope of this feature is nearly the entire API. It is justified to
throw the entire GL suite of tests at it on a regular basis. The cost
of running this once per week ought to be reasonable.

Is the cost of running this per check-in too high for the system
today? Or, is it that you think it is too high for the feature? I'm
sympathetic to the former, and not so much to the later. :)

By the way, enabling these in CI has been helpful in highlighting a
few corner case issues. So, even if you don't like it, *Thank You* for
enabling it. :)

> Beyond poor return on execution time, there is the simple fact that
> whoever is running the CI needs to manually look at shader cache results
> separately from the rest of the tests.  Unit testing is effective
> because coverage can be added at approximately zero marginal cost.
> 
> 3 years from now, will we still be looking at separate weekly shader
> cache test runs to make sure it's working?

I actually think that long term this should become part of the main
daily and weekly tests. (An idea you've already rejected.) In the long
term it should be stable enough for that, and if it does ever regress,
we'd what to see something sooner rather than later.

> > These tests are already identifying some corner case issues. I'm not
> > sure these would have impacted real-world apps yet, but I do think it
> > is a good idea to run these tests regularly.
> >
> > You say this test strategy is inadequate. Perhaps. I do think it needs
> > to be part of our test strategy. There is no way we are going to hit
> > all the corners of the API better than running all of our tests with
> > the cache enabled. Do you agree?
> 
> Tests should strive to cover the implementation, not the API.

I don't think any of our test suites operate this way. Very little of
piglit or deqp focus on i965.

> Shader
> cache is a unique feature, because it affects a large part of the API.
> It doesn't necessarily follow that covering the API will cover the
> feature, or that that is an effective test strategy.

As you say, the scope of the feature is nearly the entire API. You
want us to start building a test suite that assumes the current
implementation details of the i965 shader cache, and tests it?

Regardless of the choice to focus only on i965, the scope will still
be enormous. We will never feasibly reach the coverage level that we
get from enabling the shader cache paths, and running our current
hundreds of thousands of test cases on it.

> > It could be interesting to define a MESA extension to control or query
> > the shader cache. Today, the shader cache is a nearly 'invisible'
> > feature. There are a few env-vars, but I wouldn't call a public
> > interface.
> >
> > The downside of defining an extension is that it might constrain
> > changes to the shader cache implementation. Or, if the interface is
> > too complex, then it may be tough for some drivers to implement.
> 
> Why is an extension necessary?  By comparison, GCC has no interface to
> query the statistics for ccache.  A utility that can read the cache
> files and report hits/misses would at least inform us that the cache is
> functioning.  The utility would be useful in writing real unit tests for
> the feature.

If we don't have an extension, then how would this work? i965 unit
tests during the build? If we don't actually test the feature against
real hardware, I think we'll miss a whole class of potential issues.

If we don't have an extension, then how can an external (piglit) test
hope to interact or probe the cache?

I'm not actually in favor of defining and implementing an extension,
because I'm not sure about the ROI.

> > But, what if we get around to defining and implementing this extenion,
> > (and a few piglit tests that makes use of it), by mid-January? It'll
> > still be pretty difficult to argue we should enable the shader cache
> > on i965 for 18.0, since we burned all the time to let it be tested on
> > master against real-world apps.
> 
> Personally, I think it is already difficult to argue that the cache
> should be on by default in 18.0.

Ok. I guess I'll check back with you sometime in 2018...

> > Also, while you say our current test strategy is inadequate, I do
> > think it is still far more comprehensive than anything we will
> > accomplish by defining a new extension and writing a few, or even a
> > few hundred tests against the new extension.
> 
> In the short term, we should do whatever is expedient to try to test the
> feature as much as we can.
> 
> In the longer term, a weekly test is going to be costly and ineffective
> at preventing regressions.  Tests need to be runnable for developers to
> use them as part of their process.  If a test can only be run at great
> expense in a weekly CI, then any fragility in the feature will be
> discovered a week after a bug has been pushed to master.

Given the scope of the feature, I just don't think it is going to be
possible to get reasonable coverage on a developer's machine.

Once an issue has been identified, it has generally been easy to
reproduce locally.

-Jordan
Jordan Justen <jordan.l.justen@intel.com> writes:

> On 2017-12-05 14:49:28, Mark Janes wrote:
>> Jordan Justen <jordan.l.justen@intel.com> writes:
>> 
>> > On 2017-12-05 09:13:11, Mark Janes wrote:
>> >> Jordan Justen <jordan.l.justen@intel.com> writes:
>> >> 
>> >> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
>> >> >> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>> >> >> 
>> >> >> Mark may want to consider adding some of the once a day type CI runs for 
>> >> >> this. For example running the test suite for two consecutive runs on the 
>> >> >> same build so that the second run uses the shader cache and also a 
>> >> >> second run the uses MESA_GLSL=cache_fb to force testing of the cache 
>> >> >> fallback path.
>> >> >
>> >> > Yeah. We discussed this previously, but I don't think it's been
>> >> > implemented yet. My opinion is that it could perhaps be a weekly test.
>> >> 
>> >> This automation is implemented now. It found the issues reported in
>> >> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>> >> 
>> >> My opinion is that this test strategy is inadequate.
>> >
>> > Meaning you think we cannot enable i965 shader cache for the 18.0
>> > release?
>> 
>> I haven't heard anyone express confidence that this feature is bug-free,
>> and I don't know on what basis that claim could be made.  I appreciate
>> that a lot of have manual testing has been done.  Do you feel confident
>> that the cache can be enabled for all mesa customers without disruption?
>
> If we had enabled it two months ago, then yes, we would have worked
> through the niche issues, or discovered the so-far-hidden major land
> mine. At this point, it's getting risky. In a month, I will say no.
>
>> > We are already ~1.5 months away from the next stable branch point. If
>> > we want to enable this in 18.0, we should be using this time to see if
>> > enabling the cache by default has some large unexpected side effect in
>> > our graphics stack, or breaks real-world apps.
>> 
>> I agree.  We should encourage as many users as possible to enable the
>> shader cache in their environments.  At least one stable release should
>> be out with a working cache, where the feature can be enabled by those
>> who are eager to benefit from it.  I assume there will be a lot of them,
>> and they could flush out issues for everyone who has no idea what a
>> shader cache is.
>> 
>> >> Adding a dimension to the test matrix has high cost, especially when
>> >> combined with other dimensions of the test matrix (does shader cache
>> >> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>> >
>> > Are you saying this is too high cost to run per check-in? Do you need
>> > to disable it for the health of CI? I think I proposed that daily, or
>> > perhaps even weekly would be adequate.
>> 
>> Certainly, the test time per line of shader cache code is massively
>> higher than any other feature, even if you run it just once a month.
>> Other features have tests that run in milliseconds, not 30min * 20
>> machines.
>
> The scope of this feature is nearly the entire API. It is justified to
> throw the entire GL suite of tests at it on a regular basis. The cost
> of running this once per week ought to be reasonable.
>
> Is the cost of running this per check-in too high for the system
> today? Or, is it that you think it is too high for the feature? I'm
> sympathetic to the former, and not so much to the later. :)
>
> By the way, enabling these in CI has been helpful in highlighting a
> few corner case issues. So, even if you don't like it, *Thank You* for
> enabling it. :)
>
>> Beyond poor return on execution time, there is the simple fact that
>> whoever is running the CI needs to manually look at shader cache results
>> separately from the rest of the tests.  Unit testing is effective
>> because coverage can be added at approximately zero marginal cost.
>> 
>> 3 years from now, will we still be looking at separate weekly shader
>> cache test runs to make sure it's working?
>
> I actually think that long term this should become part of the main
> daily and weekly tests. (An idea you've already rejected.) In the long
> term it should be stable enough for that, and if it does ever regress,
> we'd what to see something sooner rather than later.
>
>> > These tests are already identifying some corner case issues. I'm not
>> > sure these would have impacted real-world apps yet, but I do think it
>> > is a good idea to run these tests regularly.
>> >
>> > You say this test strategy is inadequate. Perhaps. I do think it needs
>> > to be part of our test strategy. There is no way we are going to hit
>> > all the corners of the API better than running all of our tests with
>> > the cache enabled. Do you agree?
>> 
>> Tests should strive to cover the implementation, not the API.
>
> I don't think any of our test suites operate this way. Very little of
> piglit or deqp focus on i965.
>
>> Shader
>> cache is a unique feature, because it affects a large part of the API.
>> It doesn't necessarily follow that covering the API will cover the
>> feature, or that that is an effective test strategy.
>
> As you say, the scope of the feature is nearly the entire API. You
> want us to start building a test suite that assumes the current
> implementation details of the i965 shader cache, and tests it?
>
> Regardless of the choice to focus only on i965, the scope will still
> be enormous. We will never feasibly reach the coverage level that we
> get from enabling the shader cache paths, and running our current
> hundreds of thousands of test cases on it.
>
>> > It could be interesting to define a MESA extension to control or query
>> > the shader cache. Today, the shader cache is a nearly 'invisible'
>> > feature. There are a few env-vars, but I wouldn't call a public
>> > interface.
>> >
>> > The downside of defining an extension is that it might constrain
>> > changes to the shader cache implementation. Or, if the interface is
>> > too complex, then it may be tough for some drivers to implement.
>> 
>> Why is an extension necessary?  By comparison, GCC has no interface to
>> query the statistics for ccache.  A utility that can read the cache
>> files and report hits/misses would at least inform us that the cache is
>> functioning.  The utility would be useful in writing real unit tests for
>> the feature.
>
> If we don't have an extension, then how would this work? i965 unit
> tests during the build? If we don't actually test the feature against
> real hardware, I think we'll miss a whole class of potential issues.

Unit tests are good, and I know we already have some of those.  We'll
have better data on the classes of issues are once we find more of
them.  So far, it's just a few hardware-independent piglit tests.

> If we don't have an extension, then how can an external (piglit) test
> hope to interact or probe the cache?

Piglit queries a utility or lib to make assertions about the cache?  For
that matter, this whole cache verification could be implemented within
piglit as a profile that executes tests twice with appropriate
verification.

Then, at least, developers could test their patches without a CI.
Piglit could even skip a bunch of tests which don't impact the cache.

> I'm not actually in favor of defining and implementing an extension,
> because I'm not sure about the ROI.
>
>> > But, what if we get around to defining and implementing this extenion,
>> > (and a few piglit tests that makes use of it), by mid-January? It'll
>> > still be pretty difficult to argue we should enable the shader cache
>> > on i965 for 18.0, since we burned all the time to let it be tested on
>> > master against real-world apps.
>> 
>> Personally, I think it is already difficult to argue that the cache
>> should be on by default in 18.0.
>
> Ok. I guess I'll check back with you sometime in 2018...
>
>> > Also, while you say our current test strategy is inadequate, I do
>> > think it is still far more comprehensive than anything we will
>> > accomplish by defining a new extension and writing a few, or even a
>> > few hundred tests against the new extension.
>> 
>> In the short term, we should do whatever is expedient to try to test the
>> feature as much as we can.
>> 
>> In the longer term, a weekly test is going to be costly and ineffective
>> at preventing regressions.  Tests need to be runnable for developers to
>> use them as part of their process.  If a test can only be run at great
>> expense in a weekly CI, then any fragility in the feature will be
>> discovered a week after a bug has been pushed to master.
>
> Given the scope of the feature, I just don't think it is going to be
> possible to get reasonable coverage on a developer's machine.
>
> Once an issue has been identified, it has generally been easy to
> reproduce locally.
>
> -Jordan
On 06/12/17 12:04, Mark Janes wrote:
> Jordan Justen <jordan.l.justen@intel.com> writes:
> 
>> On 2017-12-05 14:49:28, Mark Janes wrote:
>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>
>>>> On 2017-12-05 09:13:11, Mark Janes wrote:
>>>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>>>
>>>>>> On 2017-11-08 17:26:47, Timothy Arceri wrote:
>>>>>>> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>>>>>>>
>>>>>>> Mark may want to consider adding some of the once a day type CI runs for
>>>>>>> this. For example running the test suite for two consecutive runs on the
>>>>>>> same build so that the second run uses the shader cache and also a
>>>>>>> second run the uses MESA_GLSL=cache_fb to force testing of the cache
>>>>>>> fallback path.
>>>>>>
>>>>>> Yeah. We discussed this previously, but I don't think it's been
>>>>>> implemented yet. My opinion is that it could perhaps be a weekly test.
>>>>>
>>>>> This automation is implemented now. It found the issues reported in
>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>>>>>
>>>>> My opinion is that this test strategy is inadequate.
>>>>
>>>> Meaning you think we cannot enable i965 shader cache for the 18.0
>>>> release?
>>>
>>> I haven't heard anyone express confidence that this feature is bug-free,
>>> and I don't know on what basis that claim could be made.  I appreciate
>>> that a lot of have manual testing has been done.  Do you feel confident
>>> that the cache can be enabled for all mesa customers without disruption?
>>
>> If we had enabled it two months ago, then yes, we would have worked
>> through the niche issues, or discovered the so-far-hidden major land
>> mine. At this point, it's getting risky. In a month, I will say no.
>>
>>>> We are already ~1.5 months away from the next stable branch point. If
>>>> we want to enable this in 18.0, we should be using this time to see if
>>>> enabling the cache by default has some large unexpected side effect in
>>>> our graphics stack, or breaks real-world apps.
>>>
>>> I agree.  We should encourage as many users as possible to enable the
>>> shader cache in their environments.  At least one stable release should
>>> be out with a working cache, where the feature can be enabled by those
>>> who are eager to benefit from it.  I assume there will be a lot of them,
>>> and they could flush out issues for everyone who has no idea what a
>>> shader cache is.
>>>
>>>>> Adding a dimension to the test matrix has high cost, especially when
>>>>> combined with other dimensions of the test matrix (does shader cache
>>>>> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>>>>
>>>> Are you saying this is too high cost to run per check-in? Do you need
>>>> to disable it for the health of CI? I think I proposed that daily, or
>>>> perhaps even weekly would be adequate.
>>>
>>> Certainly, the test time per line of shader cache code is massively
>>> higher than any other feature, even if you run it just once a month.
>>> Other features have tests that run in milliseconds, not 30min * 20
>>> machines.
>>
>> The scope of this feature is nearly the entire API. It is justified to
>> throw the entire GL suite of tests at it on a regular basis. The cost
>> of running this once per week ought to be reasonable.
>>
>> Is the cost of running this per check-in too high for the system
>> today? Or, is it that you think it is too high for the feature? I'm
>> sympathetic to the former, and not so much to the later. :)
>>
>> By the way, enabling these in CI has been helpful in highlighting a
>> few corner case issues. So, even if you don't like it, *Thank You* for
>> enabling it. :)
>>
>>> Beyond poor return on execution time, there is the simple fact that
>>> whoever is running the CI needs to manually look at shader cache results
>>> separately from the rest of the tests.  Unit testing is effective
>>> because coverage can be added at approximately zero marginal cost.
>>>
>>> 3 years from now, will we still be looking at separate weekly shader
>>> cache test runs to make sure it's working?
>>
>> I actually think that long term this should become part of the main
>> daily and weekly tests. (An idea you've already rejected.) In the long
>> term it should be stable enough for that, and if it does ever regress,
>> we'd what to see something sooner rather than later.
>>
>>>> These tests are already identifying some corner case issues. I'm not
>>>> sure these would have impacted real-world apps yet, but I do think it
>>>> is a good idea to run these tests regularly.
>>>>
>>>> You say this test strategy is inadequate. Perhaps. I do think it needs
>>>> to be part of our test strategy. There is no way we are going to hit
>>>> all the corners of the API better than running all of our tests with
>>>> the cache enabled. Do you agree?
>>>
>>> Tests should strive to cover the implementation, not the API.
>>
>> I don't think any of our test suites operate this way. Very little of
>> piglit or deqp focus on i965.
>>
>>> Shader
>>> cache is a unique feature, because it affects a large part of the API.
>>> It doesn't necessarily follow that covering the API will cover the
>>> feature, or that that is an effective test strategy.
>>
>> As you say, the scope of the feature is nearly the entire API. You
>> want us to start building a test suite that assumes the current
>> implementation details of the i965 shader cache, and tests it?
>>
>> Regardless of the choice to focus only on i965, the scope will still
>> be enormous. We will never feasibly reach the coverage level that we
>> get from enabling the shader cache paths, and running our current
>> hundreds of thousands of test cases on it.
>>
>>>> It could be interesting to define a MESA extension to control or query
>>>> the shader cache. Today, the shader cache is a nearly 'invisible'
>>>> feature. There are a few env-vars, but I wouldn't call a public
>>>> interface.
>>>>
>>>> The downside of defining an extension is that it might constrain
>>>> changes to the shader cache implementation. Or, if the interface is
>>>> too complex, then it may be tough for some drivers to implement.
>>>
>>> Why is an extension necessary?  By comparison, GCC has no interface to
>>> query the statistics for ccache.  A utility that can read the cache
>>> files and report hits/misses would at least inform us that the cache is
>>> functioning.  The utility would be useful in writing real unit tests for
>>> the feature.
>>
>> If we don't have an extension, then how would this work? i965 unit
>> tests during the build? If we don't actually test the feature against
>> real hardware, I think we'll miss a whole class of potential issues.
> 
> Unit tests are good, and I know we already have some of those.  We'll
> have better data on the classes of issues are once we find more of
> them.  So far, it's just a few hardware-independent piglit tests.
> 
>> If we don't have an extension, then how can an external (piglit) test
>> hope to interact or probe the cache?
> 
> Piglit queries a utility or lib to make assertions about the cache?  For
> that matter, this whole cache verification could be implemented within
> piglit as a profile that executes tests twice with appropriate
> verification.

That would mean manual work to add tests to a profile. The manual part 
makes it unlikely to ever happen, also trying to manually update a list 
to test every variation of shader is doomed to fail. Alternatively 
adding only tests that have found a problem in the past makes it not 
very useful.

> 
> Then, at least, developers could test their patches without a CI.
> Piglit could even skip a bunch of tests which don't impact the cache.

They already can, they just need to run piglit twice. If thats not easy 
enough we can add a profile that causes everything to be run twice.


> 
>> I'm not actually in favor of defining and implementing an extension,
>> because I'm not sure about the ROI.
>>
>>>> But, what if we get around to defining and implementing this extenion,
>>>> (and a few piglit tests that makes use of it), by mid-January? It'll
>>>> still be pretty difficult to argue we should enable the shader cache
>>>> on i965 for 18.0, since we burned all the time to let it be tested on
>>>> master against real-world apps.
>>>
>>> Personally, I think it is already difficult to argue that the cache
>>> should be on by default in 18.0.
>>
>> Ok. I guess I'll check back with you sometime in 2018...
>>
>>>> Also, while you say our current test strategy is inadequate, I do
>>>> think it is still far more comprehensive than anything we will
>>>> accomplish by defining a new extension and writing a few, or even a
>>>> few hundred tests against the new extension.
>>>
>>> In the short term, we should do whatever is expedient to try to test the
>>> feature as much as we can.
>>>
>>> In the longer term, a weekly test is going to be costly and ineffective
>>> at preventing regressions.  Tests need to be runnable for developers to
>>> use them as part of their process.  If a test can only be run at great
>>> expense in a weekly CI, then any fragility in the feature will be
>>> discovered a week after a bug has been pushed to master.
>>
>> Given the scope of the feature, I just don't think it is going to be
>> possible to get reasonable coverage on a developer's machine.
>>
>> Once an issue has been identified, it has generally been easy to
>> reproduce locally.
>>
>> -Jordan
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Timothy Arceri <tarceri@itsqueeze.com> writes:

> On 06/12/17 12:04, Mark Janes wrote:
>> Jordan Justen <jordan.l.justen@intel.com> writes:
>> 
>>> On 2017-12-05 14:49:28, Mark Janes wrote:
>>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>>
>>>>> On 2017-12-05 09:13:11, Mark Janes wrote:
>>>>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>>>>
>>>>>>> On 2017-11-08 17:26:47, Timothy Arceri wrote:
>>>>>>>> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>>>>>>>>
>>>>>>>> Mark may want to consider adding some of the once a day type CI runs for
>>>>>>>> this. For example running the test suite for two consecutive runs on the
>>>>>>>> same build so that the second run uses the shader cache and also a
>>>>>>>> second run the uses MESA_GLSL=cache_fb to force testing of the cache
>>>>>>>> fallback path.
>>>>>>>
>>>>>>> Yeah. We discussed this previously, but I don't think it's been
>>>>>>> implemented yet. My opinion is that it could perhaps be a weekly test.
>>>>>>
>>>>>> This automation is implemented now. It found the issues reported in
>>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>>>>>>
>>>>>> My opinion is that this test strategy is inadequate.
>>>>>
>>>>> Meaning you think we cannot enable i965 shader cache for the 18.0
>>>>> release?
>>>>
>>>> I haven't heard anyone express confidence that this feature is bug-free,
>>>> and I don't know on what basis that claim could be made.  I appreciate
>>>> that a lot of have manual testing has been done.  Do you feel confident
>>>> that the cache can be enabled for all mesa customers without disruption?
>>>
>>> If we had enabled it two months ago, then yes, we would have worked
>>> through the niche issues, or discovered the so-far-hidden major land
>>> mine. At this point, it's getting risky. In a month, I will say no.
>>>
>>>>> We are already ~1.5 months away from the next stable branch point. If
>>>>> we want to enable this in 18.0, we should be using this time to see if
>>>>> enabling the cache by default has some large unexpected side effect in
>>>>> our graphics stack, or breaks real-world apps.
>>>>
>>>> I agree.  We should encourage as many users as possible to enable the
>>>> shader cache in their environments.  At least one stable release should
>>>> be out with a working cache, where the feature can be enabled by those
>>>> who are eager to benefit from it.  I assume there will be a lot of them,
>>>> and they could flush out issues for everyone who has no idea what a
>>>> shader cache is.
>>>>
>>>>>> Adding a dimension to the test matrix has high cost, especially when
>>>>>> combined with other dimensions of the test matrix (does shader cache
>>>>>> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>>>>>
>>>>> Are you saying this is too high cost to run per check-in? Do you need
>>>>> to disable it for the health of CI? I think I proposed that daily, or
>>>>> perhaps even weekly would be adequate.
>>>>
>>>> Certainly, the test time per line of shader cache code is massively
>>>> higher than any other feature, even if you run it just once a month.
>>>> Other features have tests that run in milliseconds, not 30min * 20
>>>> machines.
>>>
>>> The scope of this feature is nearly the entire API. It is justified to
>>> throw the entire GL suite of tests at it on a regular basis. The cost
>>> of running this once per week ought to be reasonable.
>>>
>>> Is the cost of running this per check-in too high for the system
>>> today? Or, is it that you think it is too high for the feature? I'm
>>> sympathetic to the former, and not so much to the later. :)
>>>
>>> By the way, enabling these in CI has been helpful in highlighting a
>>> few corner case issues. So, even if you don't like it, *Thank You* for
>>> enabling it. :)
>>>
>>>> Beyond poor return on execution time, there is the simple fact that
>>>> whoever is running the CI needs to manually look at shader cache results
>>>> separately from the rest of the tests.  Unit testing is effective
>>>> because coverage can be added at approximately zero marginal cost.
>>>>
>>>> 3 years from now, will we still be looking at separate weekly shader
>>>> cache test runs to make sure it's working?
>>>
>>> I actually think that long term this should become part of the main
>>> daily and weekly tests. (An idea you've already rejected.) In the long
>>> term it should be stable enough for that, and if it does ever regress,
>>> we'd what to see something sooner rather than later.
>>>
>>>>> These tests are already identifying some corner case issues. I'm not
>>>>> sure these would have impacted real-world apps yet, but I do think it
>>>>> is a good idea to run these tests regularly.
>>>>>
>>>>> You say this test strategy is inadequate. Perhaps. I do think it needs
>>>>> to be part of our test strategy. There is no way we are going to hit
>>>>> all the corners of the API better than running all of our tests with
>>>>> the cache enabled. Do you agree?
>>>>
>>>> Tests should strive to cover the implementation, not the API.
>>>
>>> I don't think any of our test suites operate this way. Very little of
>>> piglit or deqp focus on i965.
>>>
>>>> Shader
>>>> cache is a unique feature, because it affects a large part of the API.
>>>> It doesn't necessarily follow that covering the API will cover the
>>>> feature, or that that is an effective test strategy.
>>>
>>> As you say, the scope of the feature is nearly the entire API. You
>>> want us to start building a test suite that assumes the current
>>> implementation details of the i965 shader cache, and tests it?
>>>
>>> Regardless of the choice to focus only on i965, the scope will still
>>> be enormous. We will never feasibly reach the coverage level that we
>>> get from enabling the shader cache paths, and running our current
>>> hundreds of thousands of test cases on it.
>>>
>>>>> It could be interesting to define a MESA extension to control or query
>>>>> the shader cache. Today, the shader cache is a nearly 'invisible'
>>>>> feature. There are a few env-vars, but I wouldn't call a public
>>>>> interface.
>>>>>
>>>>> The downside of defining an extension is that it might constrain
>>>>> changes to the shader cache implementation. Or, if the interface is
>>>>> too complex, then it may be tough for some drivers to implement.
>>>>
>>>> Why is an extension necessary?  By comparison, GCC has no interface to
>>>> query the statistics for ccache.  A utility that can read the cache
>>>> files and report hits/misses would at least inform us that the cache is
>>>> functioning.  The utility would be useful in writing real unit tests for
>>>> the feature.
>>>
>>> If we don't have an extension, then how would this work? i965 unit
>>> tests during the build? If we don't actually test the feature against
>>> real hardware, I think we'll miss a whole class of potential issues.
>> 
>> Unit tests are good, and I know we already have some of those.  We'll
>> have better data on the classes of issues are once we find more of
>> them.  So far, it's just a few hardware-independent piglit tests.
>> 
>>> If we don't have an extension, then how can an external (piglit) test
>>> hope to interact or probe the cache?
>> 
>> Piglit queries a utility or lib to make assertions about the cache?  For
>> that matter, this whole cache verification could be implemented within
>> piglit as a profile that executes tests twice with appropriate
>> verification.
>
> That would mean manual work to add tests to a profile. The manual part 
> makes it unlikely to ever happen, also trying to manually update a list 
> to test every variation of shader is doomed to fail. Alternatively 
> adding only tests that have found a problem in the past makes it not 
> very useful.

Are there not some cpu tests that can be easily dropped from the
profile?

>> 
>> Then, at least, developers could test their patches without a CI.
>> Piglit could even skip a bunch of tests which don't impact the cache.
>
> They already can, they just need to run piglit twice. If thats not easy 
> enough we can add a profile that causes everything to be run twice.
>
>> 
>>> I'm not actually in favor of defining and implementing an extension,
>>> because I'm not sure about the ROI.
>>>
>>>>> But, what if we get around to defining and implementing this extenion,
>>>>> (and a few piglit tests that makes use of it), by mid-January? It'll
>>>>> still be pretty difficult to argue we should enable the shader cache
>>>>> on i965 for 18.0, since we burned all the time to let it be tested on
>>>>> master against real-world apps.
>>>>
>>>> Personally, I think it is already difficult to argue that the cache
>>>> should be on by default in 18.0.
>>>
>>> Ok. I guess I'll check back with you sometime in 2018...
>>>
>>>>> Also, while you say our current test strategy is inadequate, I do
>>>>> think it is still far more comprehensive than anything we will
>>>>> accomplish by defining a new extension and writing a few, or even a
>>>>> few hundred tests against the new extension.
>>>>
>>>> In the short term, we should do whatever is expedient to try to test the
>>>> feature as much as we can.
>>>>
>>>> In the longer term, a weekly test is going to be costly and ineffective
>>>> at preventing regressions.  Tests need to be runnable for developers to
>>>> use them as part of their process.  If a test can only be run at great
>>>> expense in a weekly CI, then any fragility in the feature will be
>>>> discovered a week after a bug has been pushed to master.
>>>
>>> Given the scope of the feature, I just don't think it is going to be
>>> possible to get reasonable coverage on a developer's machine.
>>>
>>> Once an issue has been identified, it has generally been easy to
>>> reproduce locally.
>>>
>>> -Jordan
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
On 2017-12-05 18:30:30, Mark Janes wrote:
> Timothy Arceri <tarceri@itsqueeze.com> writes:
> 
> > On 06/12/17 12:04, Mark Janes wrote:
> >> Jordan Justen <jordan.l.justen@intel.com> writes:
> >> 
> >>> On 2017-12-05 14:49:28, Mark Janes wrote:
> >>>> Jordan Justen <jordan.l.justen@intel.com> writes:
> >>>>> It could be interesting to define a MESA extension to control or query
> >>>>> the shader cache. Today, the shader cache is a nearly 'invisible'
> >>>>> feature. There are a few env-vars, but I wouldn't call a public
> >>>>> interface.
> >>>>>
> >>>>> The downside of defining an extension is that it might constrain
> >>>>> changes to the shader cache implementation. Or, if the interface is
> >>>>> too complex, then it may be tough for some drivers to implement.
> >>>>
> >>>> Why is an extension necessary?  By comparison, GCC has no interface to
> >>>> query the statistics for ccache.  A utility that can read the cache
> >>>> files and report hits/misses would at least inform us that the cache is
> >>>> functioning.  The utility would be useful in writing real unit tests for
> >>>> the feature.
> >>>
> >>> If we don't have an extension, then how would this work? i965 unit
> >>> tests during the build? If we don't actually test the feature against
> >>> real hardware, I think we'll miss a whole class of potential issues.
> >> 
> >> Unit tests are good, and I know we already have some of those.  We'll
> >> have better data on the classes of issues are once we find more of
> >> them.  So far, it's just a few hardware-independent piglit tests.
> >> 
> >>> If we don't have an extension, then how can an external (piglit) test
> >>> hope to interact or probe the cache?
> >> 
> >> Piglit queries a utility or lib to make assertions about the cache?  For
> >> that matter, this whole cache verification could be implemented within
> >> piglit as a profile that executes tests twice with appropriate
> >> verification.
> >
> > That would mean manual work to add tests to a profile. The manual part 
> > makes it unlikely to ever happen, also trying to manually update a list 
> > to test every variation of shader is doomed to fail. Alternatively 
> > adding only tests that have found a problem in the past makes it not 
> > very useful.
> 
> Are there not some cpu tests that can be easily dropped from the
> profile?

Looking in tests/cpu.py, I guess the asmparsertests are probably not
affected. (At least today, and that's unlikely to change.)

For the GLSLParserTest tests, they do interact with the shader cache,
so they should still be run.

Uh, not sure if I mentioned it previously, but the shader cache has no
need to run any vulkan tests.

-Jordan
On Tue, Dec 5, 2017 at 3:40 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2017-12-05 14:49:28, Mark Janes wrote:
>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>
>> > On 2017-12-05 09:13:11, Mark Janes wrote:
>> >> Jordan Justen <jordan.l.justen@intel.com> writes:
>> >>
>> >> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
>> >> >> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>> >> >>
>> >> >> Mark may want to consider adding some of the once a day type CI runs for
>> >> >> this. For example running the test suite for two consecutive runs on the
>> >> >> same build so that the second run uses the shader cache and also a
>> >> >> second run the uses MESA_GLSL=cache_fb to force testing of the cache
>> >> >> fallback path.
>> >> >
>> >> > Yeah. We discussed this previously, but I don't think it's been
>> >> > implemented yet. My opinion is that it could perhaps be a weekly test.
>> >>
>> >> This automation is implemented now. It found the issues reported in
>> >> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>> >>
>> >> My opinion is that this test strategy is inadequate.
>> >
>> > Meaning you think we cannot enable i965 shader cache for the 18.0
>> > release?
>>
>> I haven't heard anyone express confidence that this feature is bug-free,
>> and I don't know on what basis that claim could be made.  I appreciate
>> that a lot of have manual testing has been done.  Do you feel confident
>> that the cache can be enabled for all mesa customers without disruption?
>
> If we had enabled it two months ago, then yes, we would have worked
> through the niche issues, or discovered the so-far-hidden major land
> mine. At this point, it's getting risky. In a month, I will say no.
>
>> > We are already ~1.5 months away from the next stable branch point. If
>> > we want to enable this in 18.0, we should be using this time to see if
>> > enabling the cache by default has some large unexpected side effect in
>> > our graphics stack, or breaks real-world apps.
>>
>> I agree.  We should encourage as many users as possible to enable the
>> shader cache in their environments.  At least one stable release should
>> be out with a working cache, where the feature can be enabled by those
>> who are eager to benefit from it.  I assume there will be a lot of them,
>> and they could flush out issues for everyone who has no idea what a
>> shader cache is.
>>
>> >> Adding a dimension to the test matrix has high cost, especially when
>> >> combined with other dimensions of the test matrix (does shader cache
>> >> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>> >
>> > Are you saying this is too high cost to run per check-in? Do you need
>> > to disable it for the health of CI? I think I proposed that daily, or
>> > perhaps even weekly would be adequate.
>>
>> Certainly, the test time per line of shader cache code is massively
>> higher than any other feature, even if you run it just once a month.
>> Other features have tests that run in milliseconds, not 30min * 20
>> machines.
>
> The scope of this feature is nearly the entire API. It is justified to
> throw the entire GL suite of tests at it on a regular basis. The cost
> of running this once per week ought to be reasonable.

But the entire API boils down to a comparatively small set of
non-orthogonal state. The configuration of those nobs seems to me like
the place things are most likely to break.

I think there's value in testing that we're hitting the cache, but if
we're not it's not a functional regression. I'm more concerned about
ensuring we don't have bugs that affect functionality and cause things
to break.

The program key for fragment shaders looks like:

/** The program key for Fragment/Pixel Shaders. */
struct brw_wm_prog_key {
   /* Some collection of BRW_WM_IZ_* */
   uint8_t iz_lookup;
   bool stats_wm:1;
   bool flat_shade:1;
   unsigned nr_color_regions:5;
   bool replicate_alpha:1;
   bool clamp_fragment_color:1;
   bool persample_interp:1;
   bool multisample_fbo:1;
   bool frag_coord_adds_sample_pos:1;
   enum brw_wm_aa_enable line_aa:2;
   bool high_quality_derivatives:1;
   bool force_dual_color_blend:1;
   bool coherent_fb_fetch:1;

   uint64_t input_slots_valid;
   unsigned program_string_id;
   GLenum alpha_test_func;          /* < For Gen4/5 MRT alpha test */
   float alpha_test_ref;

   struct brw_sampler_prog_key_data tex;
};

and it's the most complex of the shader stages. Wouldn't you feel a
lot safer if we just had a piglit test for each of those nobs that
compiled a shader, then changed some non-orthogonal state, and then
rendered with it, thus confirming that it didn't get the wrong shader
program out of the cache?

I know I've run into cases numerous times where piglit doesn't
actually test something, or only one seemingly unrelated test in all
50 thousand tickles a real bug in my code. I feel uncomfortable
assuming that piglit's existing coverage is good enough.

In general, I'm still bothered by the idea of just running piglit
twice. That's a very untargeted approach that doesn't fit well with
our existing testing model, as Mark has described.
On 08/12/17 04:57, Matt Turner wrote:
> On Tue, Dec 5, 2017 at 3:40 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:
>> On 2017-12-05 14:49:28, Mark Janes wrote:
>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>
>>>> On 2017-12-05 09:13:11, Mark Janes wrote:
>>>>> Jordan Justen <jordan.l.justen@intel.com> writes:
>>>>>
>>>>>> On 2017-11-08 17:26:47, Timothy Arceri wrote:
>>>>>>> Reviewed-by: Timothy Arceri <tarceri@itsqueeze.com>
>>>>>>>
>>>>>>> Mark may want to consider adding some of the once a day type CI runs for
>>>>>>> this. For example running the test suite for two consecutive runs on the
>>>>>>> same build so that the second run uses the shader cache and also a
>>>>>>> second run the uses MESA_GLSL=cache_fb to force testing of the cache
>>>>>>> fallback path.
>>>>>>
>>>>>> Yeah. We discussed this previously, but I don't think it's been
>>>>>> implemented yet. My opinion is that it could perhaps be a weekly test.
>>>>>
>>>>> This automation is implemented now. It found the issues reported in
>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=103988
>>>>>
>>>>> My opinion is that this test strategy is inadequate.
>>>>
>>>> Meaning you think we cannot enable i965 shader cache for the 18.0
>>>> release?
>>>
>>> I haven't heard anyone express confidence that this feature is bug-free,
>>> and I don't know on what basis that claim could be made.  I appreciate
>>> that a lot of have manual testing has been done.  Do you feel confident
>>> that the cache can be enabled for all mesa customers without disruption?
>>
>> If we had enabled it two months ago, then yes, we would have worked
>> through the niche issues, or discovered the so-far-hidden major land
>> mine. At this point, it's getting risky. In a month, I will say no.
>>
>>>> We are already ~1.5 months away from the next stable branch point. If
>>>> we want to enable this in 18.0, we should be using this time to see if
>>>> enabling the cache by default has some large unexpected side effect in
>>>> our graphics stack, or breaks real-world apps.
>>>
>>> I agree.  We should encourage as many users as possible to enable the
>>> shader cache in their environments.  At least one stable release should
>>> be out with a working cache, where the feature can be enabled by those
>>> who are eager to benefit from it.  I assume there will be a lot of them,
>>> and they could flush out issues for everyone who has no idea what a
>>> shader cache is.
>>>
>>>>> Adding a dimension to the test matrix has high cost, especially when
>>>>> combined with other dimensions of the test matrix (does shader cache
>>>>> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
>>>>
>>>> Are you saying this is too high cost to run per check-in? Do you need
>>>> to disable it for the health of CI? I think I proposed that daily, or
>>>> perhaps even weekly would be adequate.
>>>
>>> Certainly, the test time per line of shader cache code is massively
>>> higher than any other feature, even if you run it just once a month.
>>> Other features have tests that run in milliseconds, not 30min * 20
>>> machines.
>>
>> The scope of this feature is nearly the entire API. It is justified to
>> throw the entire GL suite of tests at it on a regular basis. The cost
>> of running this once per week ought to be reasonable.
> 
> But the entire API boils down to a comparatively small set of
> non-orthogonal state. The configuration of those nobs seems to me like
> the place things are most likely to break.
> 
> I think there's value in testing that we're hitting the cache, but if
> we're not it's not a functional regression. I'm more concerned about
> ensuring we don't have bugs that affect functionality and cause things
> to break.
> 
> The program key for fragment shaders looks like:
> 
> /** The program key for Fragment/Pixel Shaders. */
> struct brw_wm_prog_key {
>     /* Some collection of BRW_WM_IZ_* */
>     uint8_t iz_lookup;
>     bool stats_wm:1;
>     bool flat_shade:1;
>     unsigned nr_color_regions:5;
>     bool replicate_alpha:1;
>     bool clamp_fragment_color:1;
>     bool persample_interp:1;
>     bool multisample_fbo:1;
>     bool frag_coord_adds_sample_pos:1;
>     enum brw_wm_aa_enable line_aa:2;
>     bool high_quality_derivatives:1;
>     bool force_dual_color_blend:1;
>     bool coherent_fb_fetch:1;
> 
>     uint64_t input_slots_valid;
>     unsigned program_string_id;
>     GLenum alpha_test_func;          /* < For Gen4/5 MRT alpha test */
>     float alpha_test_ref;
> 
>     struct brw_sampler_prog_key_data tex;
> };
> 
> and it's the most complex of the shader stages. Wouldn't you feel a
> lot safer if we just had a piglit test for each of those nobs that
> compiled a shader, then changed some non-orthogonal state, and then
> rendered with it, thus confirming that it didn't get the wrong shader
> program out of the cache?

Those are just the keys for i965s gen program. You also should be 
testing the serialization and deserialization of the GL state and the IR 
(in i965s case NIR). For NIR you could turn on an ENV var to do a 
serialization/deserialization for every shader, I think something like 
that is done for nir clone.

> 
> I know I've run into cases numerous times where piglit doesn't
> actually test something, or only one seemingly unrelated test in all
> 50 thousand tickles a real bug in my code. I feel uncomfortable
> assuming that piglit's existing coverage is good enough.

I don't see how this is related to the discussion, isn't this an 
existing problem? These keys are already in use for the in-memory cache.

> 
> In general, I'm still bothered by the idea of just running piglit
> twice. That's a very untargeted approach that doesn't fit well with
> our existing testing model, as Mark has described.

IMO the problem is nobody is going to be writing tests that target the 
cache, running things twice means we automatically get to make use of 
any new tests added for any new feature, without this I just see things 
breaking until someone notices it and then the bug is likely to be fix 
without a specific shader cache test being written. You say above "I 
know I've run into cases numerous times where piglit doesn't actually 
test something", while running piglit twice may be an untargeted it 
simplicity is its appeal in a test system that is already under 
resourced and has coverage gaps.
On 2017-12-07 09:57:48, Matt Turner wrote:
> On Tue, Dec 5, 2017 at 3:40 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > On 2017-12-05 14:49:28, Mark Janes wrote:
> >> Jordan Justen <jordan.l.justen@intel.com> writes:
> >> > On 2017-12-05 09:13:11, Mark Janes wrote:
> >> >> Adding a dimension to the test matrix has high cost, especially when
> >> >> combined with other dimensions of the test matrix (does shader cache
> >> >> need to be tested for 32/64 bit builds? For hswgt1/hswgt2/hswgt3e?).
> >> >
> >> > Are you saying this is too high cost to run per check-in? Do you need
> >> > to disable it for the health of CI? I think I proposed that daily, or
> >> > perhaps even weekly would be adequate.
> >>
> >> Certainly, the test time per line of shader cache code is massively
> >> higher than any other feature, even if you run it just once a month.
> >> Other features have tests that run in milliseconds, not 30min * 20
> >> machines.
> >
> > The scope of this feature is nearly the entire API. It is justified to
> > throw the entire GL suite of tests at it on a regular basis. The cost
> > of running this once per week ought to be reasonable.
> 
> But the entire API boils down to a comparatively small set of
> non-orthogonal state. The configuration of those nobs seems to me like
> the place things are most likely to break.
> 
> I think there's value in testing that we're hitting the cache, but if
> we're not it's not a functional regression. I'm more concerned about
> ensuring we don't have bugs that affect functionality and cause things
> to break.
> 
> The program key for fragment shaders looks like:
> 
> /** The program key for Fragment/Pixel Shaders. */
> struct brw_wm_prog_key {
>    /* Some collection of BRW_WM_IZ_* */
>    uint8_t iz_lookup;
>    bool stats_wm:1;
>    bool flat_shade:1;
>    unsigned nr_color_regions:5;
>    bool replicate_alpha:1;
>    bool clamp_fragment_color:1;
>    bool persample_interp:1;
>    bool multisample_fbo:1;
>    bool frag_coord_adds_sample_pos:1;
>    enum brw_wm_aa_enable line_aa:2;
>    bool high_quality_derivatives:1;
>    bool force_dual_color_blend:1;
>    bool coherent_fb_fetch:1;
> 
>    uint64_t input_slots_valid;
>    unsigned program_string_id;
>    GLenum alpha_test_func;          /* < For Gen4/5 MRT alpha test */
>    float alpha_test_ref;
> 
>    struct brw_sampler_prog_key_data tex;
> };
> 
> and it's the most complex of the shader stages. Wouldn't you feel a
> lot safer if we just had a piglit test for each of those nobs that
> compiled a shader, then changed some non-orthogonal state, and then
> rendered with it, thus confirming that it didn't get the wrong shader
> program out of the cache?

What would that be testing? That the disk_cache returns the same
program if we give it the same hash? Shouldn't disk_cache unit tests
cover this?

The scope is also more than just covering the various GL states that
might change the i965 program keys. We also need one or more programs
that are actually affected by that key change. We also need to test
that glsl program serialization works. We also need to test that nir
serialization works.

How about maintainability? Once we develop these 50~100 tests, how do
we make sure we update them if we change the i965 program keys?

> I know I've run into cases numerous times where piglit doesn't
> actually test something, or only one seemingly unrelated test in all
> 50 thousand tickles a real bug in my code. I feel uncomfortable
> assuming that piglit's existing coverage is good enough.

This is a reasonable point. Piglit has coverage holes. The CTS has
coverage holes. dEQP has coverage holes. I think the union of all 3
probably still has coverage holes. (But hopefully a fairly small set.)

> In general, I'm still bothered by the idea of just running piglit
> twice. That's a very untargeted approach that doesn't fit well with
> our existing testing model, as Mark has described.

Are you saying that running all of our CI tests twice is a waste of
time? I think that the 'dumb run everything twice' plan actually gets
us much more coverage than we'll likely get if we try to write 50~100
tests focusing on the i965 program keys. So, I don't agree that it is
a waste of time.

I think that the 'dumb run everything twice' plan gets us almost
everything we need, and is more maintainable. If it misses something,
then I think means we don't have good test coverage of the GL feature,
and we should add that test to one of our 3 possible GL test suites.

If you are arguing that the 'dumb run everything twice' plan is
valuable, but inadequate. Fine. I concede that maybe we could try to
add more specific focused tests. I'm not sure that these tests will
actually be added anytime soon, which means that we don't have a plan
for getting the i965 shader cache enabled.

-Jordan
On Thursday, December 7, 2017 9:57:48 AM PST Matt Turner wrote:
[snip]
> But the entire API boils down to a comparatively small set of
> non-orthogonal state. The configuration of those nobs seems to me like
> the place things are most likely to break.

I do like Matt's idea of adding Piglit tests specifically for NOS items.
Even if we don't add tests for every item in a key, adding tests that
hit two variants of an item in the key would ensure that we're using the
right key, and not pulling the wrong one from the cache.

Yes, it's a problem with the existing in-memory cache, but it would also
be nice to have additional coverage of that.

Another idea I had was to extend the ARB_program_interface_query tasks
to have a mode (command line argument) that causes them to compile the
shader twice.  For example:

1. Compile the shader
2. Check program interface query properties
3. Delete the shader
4. Compile the shader again - which should be a cache hit
5. Check program interface query properties again

Most of these properties come from the IR and associated metadata.
That would be a way to verify that we get that right.  I think it would
have caught the VUE map bug relating to unify_interfaces() not getting
called when restoring from the cache.

It also sounds relatively easy on paper.

If we validated NOS, and validated PIQ, then I'd feel pretty good about
the cache working with only a single run.  The run-the-whole-suite-twice
idea is nice, but most people aren't going to do that on every single
patch (maybe daily?).  I'd like to have better coverage within a single
run.