Closed Bug 676071 Opened 13 years ago Closed 12 years ago

Use ANGLE's identifier name mapping to avoid tickling drivers with long identifiers

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [gfx.relnote.13])

Attachments

(4 files, 7 obsolete files)

ANGLE patch: use SpookyHash to generate mapped ids, instead of a monotonic identifier
22.40 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
drop ANGLE patch limiting identifiers to 250 chars
2.73 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
add a couple symbols to stdc++compat to fix linux 32bit build errors (updated)
1.03 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
use ANGLE long identifier mapping, v7
34.40 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review 
ANGLE is able to map identifiers in shaders to shorter ones, guaranteed to not exceed 32 characters. This is very useful to avoid driver bugs with long identifiers. See bug 675625 comment 17. We should use that feature, like Chrome does.
We should do that soon. Here's some info to help someone get on this quickly.

This ANGLE feature was added in

   http://code.google.com/p/angleproject/source/detail?r=619

The idea is that the ANGLE shader compiler replaces long identifiers by identifiers guaranteed to be no longer than 32 characters. The way to enable this feature is to pass SH_MAP_LONG_VARIABLE_NAMES in the compileOptions bitfield to the shader->compile() method in CompileShader. See in ANGLE sources, in Compiler.cpp, the TCompiler::compile method.

But once you do this, the identifier names in the resulting translated shaders are no longer the same as in the original WebGL source, so WebGL functions taking such identifiers, such as GetUniformLocation, GetAttribLocation and BindAttribLocation need to do the mapping before calling the actual GL function. Here's what the ANGLE developer, Mo, told me:

===BEGIN MO'S EMAIL===

It's not implemented in webkit yet, but it's hooked up in chromium already.

Look at http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/shader_translator.cc?view=markup

Function GetVariableInfo.

ShGetInfo(compiler, SH_MAPPED_NAME_MAX_LENGTH, &mapped_name_len);

Which gaves you the maximum mapped name len.  Then:

ShGetActiveAttrib(compiler, i, NULL, &size, &type, name.get(), mapped_name.get());

ShGetActiveUniform(compiler, i, NULL, &size, &type, name.get(), mapped_name.get());

name is the original long name, and mapped_name will give you the shorter name.

Let me know if you have more questions.

===END MO'S EMAIL===

Finally there is one more important thing to take care of. Currently in CompileShader we only pass to the GL the translated sources if we're on desktop OpenGL. On OpenGL ES, we pass the raw untranslated shader source. That won't work anymore. The patch for this bug must change CompileShader to always pass the translated shader source to glCompileShader.
Another thing: in bug 675625 we patched ANGLE to limit identifier length to 250 characters to work around a crash. With this, that won't be needed anymore, so this patch should be unapplied and removed, and if some WebGL test page is now passing it will have to be removed from content/canvas/test/webgl/failing_tests_*.txt
The current long name mapping doesn't handle struct field names, so you still could go over 256 to cause crash after you implemented long name mapping.
Depends on: 697333
QA Contact: canvas.webgl → bjacob
Depends on: 720438
The primary problem that this patch tries to fix is that I got a weird regression on this conformance test from enabling ANGLE long id mapping:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/invalid-passed-params.html

The two shaders in the first program in this test share a long varying name, that gets translated in two different ways (webgl_g0_xxx vs webgl_g1_xxx), resulting in a linking error.

It's mysterious why this seems to only happen in Firefox not in Chromium, but anyway, I personally prefer a hash because it will allow us to not have to keep alive the unique instance global object holding all the long id mappings, thus saving memory (even avoiding a mem leak, in the corner case where a webgl context would be constantly compiling new shaders with new identifiers).

This patch imports SpookyHash from
    http://burtleburtle.net/bob/hash/spooky.html
(public domain)

It uses a 64bit hash. If we tolerate a collision probability of 1e-12, this allows for roughly 6,000 identifiers per program. (In practice other code uses 32bit hashes, see next mozilla patch, but I wanted to get that part right to increase chances it's accepted in upstream ANGLE).
Attachment #599691 - Flags: review?(jgilbert)
Until now, as we weren't doing long id shortening, and the NVIDIA driver used on our test slaves crashes above 250-ish char identifiers, we had a ANGLE patch limiting id length to 250 chars. Now's the time to drop it.
Attachment #599692 - Flags: review?(jgilbert)
What ANGLE exposes to us is arrays of attributes and uniforms, in the ANGLE compiler object.

Upon successful ANGLE translation, we retrieve these arrays and store them in the WebGLShader object. The only tricky part here is that ANGLE exposes uniform array names with [0] appended, so we have to strip that.

The WebGLProgram class gets two new auto-ptr-to-hashtable members, one for mapping long ids to mapped ids, and one for doing the reverse mapping. The reason why these are auto-ptrs, instead of plainly storing the hashtables, is that we want to be lazy here: in particular, the reverse-map is only used by getActiveAttrib and getActiveUniform, so we avoid constructing it if we can.

Then upon successful program linking, in WebGLProgram::UpdateInfo, we drop old hashtables. Functions like getAttribLocation and getActiveAttrib access the mappings through these functions:

   MapIdentifier
   ReverseMapIdentifier
   MapCompoundIdentifier  // that one is for uniform names like x[i].y[j]

MapIdentifier and ReverseMapIdentifier will build the hashtables from the arrays stored in WebGLShaders if not already done, and will then just do a hashtable lookup. MapCompoundIdentifier parses the input string and calls MapIdentifier on appropriate tokens.
Attachment #599696 - Flags: review?(jgilbert)
New try, with uint64 instead of uint64_t to fix msvc build error:
https://tbpl.mozilla.org/?tree=Try&rev=554d0d9ceec6
New try should fix the build issues on Windows (forgot to add spooky.cpp to one ANGLE makefile) and on linux (added a couple symbols to stdc++compat.cpp):

https://tbpl.mozilla.org/?tree=Try&rev=b3dae64fde37
Comment on attachment 600084 [details] [diff] [review]
add a couple symbols to stdc++compat to fix linux 32bit build errors

Review of attachment 600084 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/stdc++compat.cpp
@@ +58,5 @@
>      template ostream& ostream::_M_insert(double);
>      template ostream& ostream::_M_insert(long);
>      template ostream& ostream::_M_insert(unsigned long);
> +    template ostream& ostream::_M_insert(int64_t);
> +    template ostream& ostream::_M_insert(uint64_t);

Any particular reason not to use long long and unsigned long long?
Attachment #600084 - Flags: review?(mh+mozilla) → review+
I was concerned that long long sometimes causes a warning (-Wlong-long) so it doesn't sound kosher, and also, I wasn't sure if long long was guaranteed to always be a different type than long. Otherwise I don't care either way.
It appears you were right, uint64 gave me horrible build errors as on 64bit systems it's the same type as unsigned long, so I got redundant template specializations. Trying long long now.
(carrying r+ from glandium)
Attachment #600084 - Attachment is obsolete: true
Attachment #600200 - Flags: review+
Updated. The main change is that we truncate the strings returned by glGetActiveUniform at first square bracket, to fix a test failure on Mac. What happens is that on desktop OpenGL, it's undefined whether uniform array names are like foo or foo[0]. When it's foo[0] we want to strip the [0] part.
Attachment #599696 - Attachment is obsolete: true
Attachment #599696 - Flags: review?(jgilbert)
Attachment #600205 - Flags: review?(jgilbert)
Alright, this new version stands a chance of working correctly on Android, where we don't use the translated shaders at all.

To this effect, it rationalizes the logic in CompileShader: see the new variables translateShaderSource and targetShaderSourceLanguage.

Also, this moves the ShCompile call further down, below the early returns, fixing bug 723261.
Attachment #600205 - Attachment is obsolete: true
Attachment #600205 - Flags: review?(jgilbert)
Attachment #600284 - Flags: review?(jgilbert)
The last thing I'm stuck on is the Windows build failures. The logs say:

   Creating library xul.lib and object xul.exp
WebGLContextGL.obj : error LNK2019: unresolved external symbol ShGetActiveUniform referenced in function "public: virtual unsigned int __cdecl mozilla::WebGLContext::CompileShader(class nsIWebGLShader *)" (?CompileShader@WebGLContext@mozilla@@UEAAIPEAVnsIWebGLShader@@@Z)
WebGLContextGL.obj : error LNK2019: unresolved external symbol ShGetActiveAttrib referenced in function "public: virtual unsigned int __cdecl mozilla::WebGLContext::CompileShader(class nsIWebGLShader *)" (?CompileShader@WebGLContext@mozilla@@UEAAIPEAVnsIWebGLShader@@@Z)
xul.dll : fatal error LNK1120: 2 unresolved externals


That doesn't make sense to me: these unresolved external symbols, ShGetActiveUniform and ShGetActiveAttrib, are declared and defined exactly in the same way, in the same files, as other symbols that we are successfully using, such as ShCompile and ShGetInfo. I don't understand what's wrong about them.
Alright, this could be explained by the fact that these 2 functions are 1 line each, hence get inlined, and MSVC might be stupid enough to allow inlining to override dllexport if I understand this page correctly: http://msdn.microsoft.com/en-us/library/78t98006%28v=vs.80%29.aspx

new try:

https://tbpl.mozilla.org/?tree=Try&rev=b2d64e5fd8f6
In bug 729952, I measured that cityhash is ~2x faster than spookyhash (for short strings, anyway).

I'm going to import cityhash asap; we might want to use it here?
For what hash size? I need 64bit here.
Also, the strings being hashed here are between 33 and 256 chars long.
Yes, for 64-bit.  (I tested for collisions using 32-bit hashes, but both cityhash and spookyhash compute a 32-bit hash by truncating a longer one, so 32-bit and 64-bit speeds are the same.)

If you give me a list of strings, I'll run a perf test with them.  Or you can check out the code at https://github.com/jlebar/hashtest and substitute your own file for the |atoms| file.  (Just run |make|.)
OK. That will be interesting to do as a follow-up. For now I'm concentrating on passing WebGL conformance tests. Also, the cost of hashing these strings with any decently fast hash function is going to be low compared to the cost of compiling the shader.
(In reply to Benoit Jacob [:bjacob] from comment #20)
> The last thing I'm stuck on is the Windows build failures. The logs say:
> 
>    Creating library xul.lib and object xul.exp
> WebGLContextGL.obj : error LNK2019: unresolved external symbol
> ShGetActiveUniform referenced in function "public: virtual unsigned int
> __cdecl mozilla::WebGLContext::CompileShader(class nsIWebGLShader *)"
> (?CompileShader@WebGLContext@mozilla@@UEAAIPEAVnsIWebGLShader@@@Z)
> WebGLContextGL.obj : error LNK2019: unresolved external symbol
> ShGetActiveAttrib referenced in function "public: virtual unsigned int
> __cdecl mozilla::WebGLContext::CompileShader(class nsIWebGLShader *)"
> (?CompileShader@WebGLContext@mozilla@@UEAAIPEAVnsIWebGLShader@@@Z)
> xul.dll : fatal error LNK1120: 2 unresolved externals
> 
> 
> That doesn't make sense to me: these unresolved external symbols,
> ShGetActiveUniform and ShGetActiveAttrib, are declared and defined exactly
> in the same way, in the same files, as other symbols that we are
> successfully using, such as ShCompile and ShGetInfo. I don't understand
> what's wrong about them.

Finally got it... the problem was that since The Great Meltdown (Bug 709193) we split out the ANGLE compiler in a separate library, and so I had to add these two symbols to layout/media/symbols.def.in

I really believed in witches at some point during debugging this.
Compared to v3, the differences are:
 - the layout/media/symbols.def.in tweaks to fix the Windows linking error
 - a fix in getActiveUniform to deal correctly with [0] at end of uniform array names, to fix the Mac oranges
 - some minor renaming/cleanup
Attachment #600284 - Attachment is obsolete: true
Attachment #600284 - Flags: review?(jgilbert)
Attachment #600675 - Flags: review?(jgilbert)
While the patch v4 allowed to pass our copy of the conformance tests, it didn't make us pass the actual 1.0.1 conformance tests: some tests had been added very recently, that we still failed.

Specifically: when a shader has uniform of struct type, ANGLE exposed all the individual struct fields inside it, like "x.y" but didn't expose the struct name x, and the field name y, individually. I believe that mapping the individual components x and y is the better approach, but that will require ANGLE changes that are better deferred until after we have 1.0.1 conformance behind us. So here is a new version of the patch adapting to the current situation, where we have only a mapping for "x.y" and not for x and y.

Regarding arrays, see this comment by Mo:
http://code.google.com/p/angleproject/issues/detail?id=299#c5
That's why we only take care of the _last_ array-indexing square brackets. The others (like the [n] in x[n].y[p]) are taken care of by ANGLE's mapping.
Attachment #600836 - Flags: review?(jgilbert)
Attachment #600675 - Attachment is obsolete: true
Attachment #600675 - Flags: review?(jgilbert)
Turns out these tests are already in the tree... not sure why I didn't see them before. New try removing them from the lists of failing tests:
https://tbpl.mozilla.org/?tree=Try&rev=742f8732db27
Accidentally pushed another patch.. new try: https://tbpl.mozilla.org/?tree=Try&rev=ebddc42e49e8
Attachment #599692 - Flags: review?(jgilbert) → review+
Comment on attachment 599691 [details] [diff] [review]
ANGLE patch: use SpookyHash to generate mapped ids, instead of a monotonic identifier

Review of attachment 599691 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: gfx/angle/src/compiler/spooky.h
@@ +33,5 @@
> +  typedef  unsigned __int16 uint16;
> +  typedef  unsigned __int8  uint8;
> +#else
> +# include <stdint.h>
> +# define INLINE inline

I know we're pulling this in from elsewhere, but we should be aware that this doesn't 'force' inlining on GCC, and we have had trouble getting GCC to inline 'obvious' things in the past.
Attachment #599691 - Flags: review?(jgilbert) → review+
Comment on attachment 600836 [details] [diff] [review]
use ANGLE long identifier mapping, v5

Review of attachment 600836 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContext.h
@@ +1700,3 @@
>  };
>  
> +static bool SplitLastSquareBracket(nsACString& string, nsCString& bracketPart)

Can we get a quick comment as to what this does?

@@ +1708,5 @@
> +    if (*s != ']')
> +        return false;
> +
> +    while (*s != '[' && s != string_start)
> +      s--;

2-space indent in a 4-space file.

@@ +1816,3 @@
>      bool IsAttribInUse(unsigned i) const { return mAttribsInUse[i]; }
>  
> +    void MapIdentifier(const nsACString& name, nsCString *mappedName) {

From other code, it looks like this deals in ASCII strings. Since we're juggling ASCII and UTF16, can we put a comment here that this should take ASCII? (1)

@@ +1842,5 @@
> +            return;
> +        }
> +
> +        // not found? return name unchanged
> +        mappedName->Assign(name);

Shouldn't this never happen?
Should we assert about this in DEBUG builds? (2)

@@ +1845,5 @@
> +        // not found? return name unchanged
> +        mappedName->Assign(name);
> +    }
> +
> +    void ReverseMapIdentifier(const nsACString& name, nsCString *reverseMappedName) {

See (1).

@@ +1873,5 @@
> +            return;
> +        }
> +
> +        // not found? return name unchanged
> +        reverseMappedName->Assign(name);

See (2).

@@ +1875,5 @@
> +
> +        // not found? return name unchanged
> +        reverseMappedName->Assign(name);
> +    }
> +// 

Leftover comment slashes?

@@ +2451,5 @@
>      NS_DECL_NSIWEBGLACTIVEINFO
>  protected:
>      WebGLint mSize;
>      WebGLenum mType;
>      nsString mName;

Since we're switching back and forth between ASCII and UTF16, can we get comments on which strings are which format?

::: content/canvas/src/WebGLContextGL.cpp
@@ +3264,5 @@
>      gl->fLinkProgram(progname);
>  
>      GLint ok;
>      gl->fGetProgramiv(progname, LOCAL_GL_LINK_STATUS, &ok);
> +    program->SetLinkStatus(ok && program->UpdateInfo());

I don't love putting this logic inside the function call, but it'll do.
I'd prefer something explicit like:
if (ok)
  ok = program->UpdateInfo();

program->SetLinkStatus(ok);

@@ +4377,5 @@
>      gl->fViewport(x, y, width, height);
>      return NS_OK;
>  }
>  
> +static void TruncateLastSquareBracket(char *string, int len)

Can I also get a one-line documentation comment for this?
It sounds like it does 'foo[*]' => 'foo', but a comment would help with skimming.

@@ +4384,5 @@
> +    if (*s != ']')
> +        return;
> +
> +    while (*s != '[' && s != string)
> +      s--;

2-space indent in 4-space file.

@@ +4386,5 @@
> +
> +    while (*s != '[' && s != string)
> +      s--;
> +
> +    if (*s != '[')

Is a mismatched bracket unusual enough to warrant a debug warning or assert?

@@ +4537,5 @@
> +                                                  nsDependentCString(mapped_name)));
> +            }
> +        }
> +
> +        if (useShaderSourceTranslation) {

Why do we exit the above if block when this check is the same? Is one of them branching on the wrong var?

This used to branch here on 'if (!go->IsGLES2())', but useShaderSourceTranslation does not replicate that functionality. Is this a purposeful change in logic?
Attachment #600836 - Flags: review?(jgilbert) → review-
Also, how much work would it be to assert for hash collisions on DEBUG builds? Or to have an option to do so?
(In reply to Jeff Gilbert [:jgilbert] from comment #35)
> @@ +1842,5 @@
> > +            return;
> > +        }
> > +
> > +        // not found? return name unchanged
> > +        mappedName->Assign(name);
> 
> Shouldn't this never happen?

This happens, in a couple of situations:
 - first (most importantly) we call this on user input, e.g. in getAttribLocation, getUniformLocation etc. So we would certainly hit that path on not-found identifier strings. Adding a comment to clarify that.
 - second, we're not always doing long identifier mapping. We would like to, but on Android the ESSL backend of the ANGLE translator has a bug that prevents us from doing so, for now (see CompileShader). That does prevent us from passing 100% of the conformance suite on Android. We need to fix that in a separate bug.
 - third, I'm not 100% sure if currently ANGLE will enumerate all identifiers, or only the long ones that got mapped, but anyway, we could well decide that we want to only store non-trivial mappings (since in the real world, 99.9% of identifiers will be <= 32 chars long and won't need mapping).

> 
> @@ +2451,5 @@
> >      NS_DECL_NSIWEBGLACTIVEINFO
> >  protected:
> >      WebGLint mSize;
> >      WebGLenum mType;
> >      nsString mName;
> 
> Since we're switching back and forth between ASCII and UTF16, can we get
> comments on which strings are which format?

Let's say that we only need to comment on nsCStrings, to clarify they're ASCII. The nsStrings are UTF-16, as usual, nothing special about them.

I've added the missing comments about that in WebGLContext.h.

> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3264,5 @@
> >      gl->fLinkProgram(progname);
> >  
> >      GLint ok;
> >      gl->fGetProgramiv(progname, LOCAL_GL_LINK_STATUS, &ok);
> > +    program->SetLinkStatus(ok && program->UpdateInfo());
> 
> I don't love putting this logic inside the function call, but it'll do.
> I'd prefer something explicit like:
> if (ok)
>   ok = program->UpdateInfo();
> 
> program->SetLinkStatus(ok);

Alright. I don't want to reuse |ok| to mean two different things, so I've dumbed it down even more.

> @@ +4386,5 @@
> > +
> > +    while (*s != '[' && s != string)
> > +      s--;
> > +
> > +    if (*s != '[')
> 
> Is a mismatched bracket unusual enough to warrant a debug warning or assert?

Here we could have an assert, because that's only called on the identifier mappings returned by ANGLE, not on user input (we don't call this from e.g. getUniformLocation). But, we do call the related SplitLastSquareBracket function on user input, so that one can't assert, and for consistency, I'd then rather not have TruncateLastSquareBracket assert either.

> 
> @@ +4537,5 @@
> > +                                                  nsDependentCString(mapped_name)));
> > +            }
> > +        }
> > +
> > +        if (useShaderSourceTranslation) {
> 
> Why do we exit the above if block when this check is the same? Is one of
> them branching on the wrong var?

No, I just shouldn't have leaved the first if() scope.

> 
> This used to branch here on 'if (!go->IsGLES2())', but
> useShaderSourceTranslation does not replicate that functionality. Is this a
> purposeful change in logic?

Yes. We used to say: "on ES platforms we don't need to translate shaders because the source is already ES". But with ANGLE identifier mapping, that's not true anymore: ANGLE identifier mapping requires modifying the shader source, so we can't just forward the original source to the GL anymore.

        Attachment #600836 -
        Attachment is obsolete: true

        Attachment #602040 -
        Flags: review?(jgilbert)

    
    

    
  


  
While investigating conformance/more/functions/uniformfArrayLen1.html test failures, it turned out that some of the failures there were required more name mapping changes!

Specifically, in getActiveUniform, we call glGetActiveUniform and then reverse-map the name to get back the original name. That's tricky because in desktop OpenGL, it's undefined whether uniform array names will be returned as "foo" or "foo[0]" so we have to append "[0]" ourselves when it's an array. The problem with this test is that it tests uniform arrays of size 1, and our way to check whether it's an array is to check for "size > 1" which breaks down in the case of arrays of size 1. It sucks that glGetActiveUniform on desktop OpenGL doesn't allow to distinguish between an array of size 1 and a non-array.

So we change approach a bit here. When retrieving the name mappings from ANGLE, we no longer truncate the final [0] of arrays. Indeed, that was losing our only reliable way to tell whether a uniform is an array.

The downside is that we now have these stupid [0] lingering around in uniform array names in our mapping, so the MapIdentifier and ReverseMapIdentifier functions got yet a bit more complicated and slower. But they're not performance critical, and that stuff is going to completely change when we do things 'the right way' in ANGLE when we have time.

('The right way' would imho be to have a mapping for long tokens, so it would be purely a parser thing and would apply right away to everything (e.g. function names too). We would then parse strings like "foo[i].bar[j]" and map the 'foo' and 'bar' parts).

        Attachment #602040 -
        Attachment is obsolete: true

        Attachment #602040 -
        Flags: review?(jgilbert)

        Attachment #602093 -
        Flags: review?(jgilbert)

    
    

    
  
Comment on attachment 602093 [details] [diff] [review]
use ANGLE long identifier mapping, v7

Review of attachment 602093 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +4514,5 @@
> +            for (int i = 0; i < num_uniforms; i++) {
> +                int length, size;
> +                ShDataType type;
> +                ShGetActiveUniform(compiler, i,
> +                                    &length, &size, &type,

Misaligned arg indenting.

        Attachment #602093 -
        Flags: review?(jgilbert) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fdea5401cf5

(the two last csets are just adjusting the lists of known failures)
Depends on: 748112
Depends on: 751643
No longer depends on: 751643
Depends on: 751643
Whiteboard: [gfx.relnote.13]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: