[Mesa-dev] [PATCH] i965: Enable disk shader cache by default

[Jordan Justen]

On 2017-12-05 14:49:28, Mark Janes wrote:
> Jordan Justen <jordan.l.justen at intel.com> writes:
> 
> > On 2017-12-05 09:13:11, Mark Janes wrote:
> >> Jordan Justen <jordan.l.justen at intel.com> writes:
> >> 
> >> > On 2017-11-08 17:26:47, Timothy Arceri wrote:
> >> >> Reviewed-by: Timothy Arceri <tarceri at 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