[Mesa-dev] [v2 6/6] mesa: OES_get_program_binary functionality

[Erik Faye-Lund]

On Wed, Nov 6, 2013 at 9:19 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 6 November 2013 11:24, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>>
>> On Wed, Nov 6, 2013 at 7:49 PM, Paul Berry <stereotype441 at gmail.com>
>> wrote:
>> > On 6 November 2013 09:35, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>> >>
>> >> Sorry for the late reply.
>> >>
>> >> On Fri, Nov 1, 2013 at 4:14 PM, Tapani <tapani.palli at intel.com> wrote:
>> >> > On 11/01/2013 03:31 PM, Erik Faye-Lund wrote:
>> >> >>
>> >> >>
>> >> >> Won't using the git-sha1 as a compatibility-criteria cause issues
>> >> >> for
>> >> >> developers with local changes? I'm not so worried about this for
>> >> >> OES_get_program_binary itself, but once the shader-cache is in place
>> >> >> it sounds like a potential source of difficult to track down
>> >> >> misbehavior...
>> >> >>>>>
>> >> >>>>>
>> >> >>>>> I agree it might be too aggressive criteria but it is hard to
>> >> >>>>> come
>> >> >>>>> up
>> >> >>>>> with
>> >> >>>>> better and as simple.
>> >> >>>>
>> >> >>>> That's not my objection. My objection is that this might give
>> >> >>>> headaches for people with local modifications to the
>> >> >>>> glsl-compiler.
>> >> >>>> Local modifications does not affect the git-sha1.
>> >> >>>
>> >> >>>
>> >> >>> For the automatic shader cache this headache could be helped a bit
>> >> >>> with a
>> >> >>> environment variable or drirc setting that can be used during
>> >> >>> development.
>> >> >>> On the other hand an automatic cache must work in a transparent way
>> >> >>> so
>> >> >>> it
>> >> >>> should be always able to recover when it fails, so one should only
>> >> >>> see
>> >> >>> it
>> >> >>> as
>> >> >>> 'slower than usual' (since recompilation/relink required) sort of
>> >> >>> behaviour.
>> >> >>> The WIP of the automatic cache I sent some time earlier also marked
>> >> >>> (renamed) these 'problematic' cached shaders so that they can be
>> >> >>> detected
>> >> >>> on
>> >> >>> further runs and cache can ignore those.
>> >> >>>
>> >> >>> I agree that it might become problematic, on the other hand it is
>> >> >>> also
>> >> >>> easy
>> >> >>> to just wipe ~/.cache/mesa and disable cache.
>> >> >>
>> >> >> That won't help for programs that maintain their own (explicit)
>> >> >> shader-cache, which was the intention of introducing binary shaders
>> >> >> to
>> >> >> OpenGL ES in the first place.
>> >> >
>> >> >
>> >> > Ok, we are talking about the extension, I thought you referred to the
>> >> > automatic caching. For extension to work, we need at least more
>> >> > Piglit
>> >> > tests
>> >> > to ensure that it does not break.
>> >>
>> >> I was actually of talking about both. But for the caching, it's
>> >> probably more forgivable, as developers probably know they changed the
>> >> compiler and can step around it by flushing the cache. Especially if
>> >> the build time gets included, like Pauls suggested.
>> >>
>> >> > Of course every time you go and touch the
>> >> > code, some functionality may break, be it this extension or something
>> >> > else.
>> >>
>> >> That's completely irrelevant here. The rejection mechanism isn't
>> >> intended to catch bugs, but to detect intentional format changes. So
>> >> let's not confuse the issue.
>> >>
>> >> > I'm not sure if Chromium, Qt or other users expect glBinaryProgram
>> >> > call
>> >> > to
>> >> > always succeed, hopefully not.
>> >>
>> >> If they do, they're buggy. But there is a chance for that, yes. But
>> >> I'm actually talking about that they might get *told* that everything
>> >> went well, and still get a broken shader. Or even a crash. Of course,
>> >> this only applies when mesa is build with local modifications, but
>> >> that happens a *lot* while debugging application issues. Perhaps bugs
>> >> start to disappear, because applications take another non-buggy
>> >> code-path? It'll probably only affect developers, and not happen in
>> >> the wild. But I still don't think that's a good excuse.
>> >>
>> >> However, by a strict reading of the spec, I don't even yhink we're
>> >> allowed to break the shaders for just any reason. The wording of the
>> >> spec is "An implementation may reject a program binary if it
>> >> determines the program binary was produced by an incompatible or
>> >> outdated version of the compiler". The way I read that, changes that
>> >> doesn't modify the compiler aren't really allowed to reject previous
>> >> shaders. While diverging from the spec on this *might* not have many
>> >> real-world implications, at the very best your solution goes against
>> >> the intention of this rejection-mechanism.
>> >
>> >
>> > We have to regard any bug fix, feature improvement, or performance
>> > optimization that's applied to the parser, ast, optimization passes, or
>> > lowering passes as constituting a change to the compiler.
>>
>> Not quite. Bug-fixes are fine; if the compiled shader was buggy,
>> you'll have a buggy binary shader, and it should be loaded with those
>> bugs intact. Feature improvements also does not matter: surely, a
>> pre-compiled shader won't use newer features. The same goes for
>> performance optimization; a program that pre-compiled a shader from an
>> old compiler surely won't expect to get a performance boost from new
>> optimization passes. If so, they'll use source shaders instead, and
>> pay the price of re-compiling. This is *exactly* what binary shaders
>> are for.
>
>
> This text from OES_get_program_binary seems to say otherwise:
>
>     Even if the cached program binary format is still valid,
> ProgramBinaryOES
>     may still fail to load the cached binary.  This is the driver's way of
>     signaling to the app that it needs to recompile and recache its program
>     binaries because there has been some important change to the online
>     compiler, such as a bug fix or a significant new optimization.

OK, yeah. You're at least partially right. The spec does say
*important change*, so I guess most bugfixes would have to be
regarded. Of course, the spec doesn't qualify what important change
means, but I'm pretty sure it doesn't mean *any* change, because then
it would have said just that :)