569976 - uniform locations need to be more strict

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

A couple of things:

- Equality of WebGLUniformLocations must be preserved; that is:

  gl.getUniformLocation(prog, "uFoo") == gl.getUniformLocation(prog, "uFoo")

Right now, we create a new object for each call, which means the above would fail.

- Uniform locations need to be invalidated whenever a program is linked.

Easiest way to accomplish both of these would be to store a bunch of uniform information on WebGLProgram, potentially including a string -> location object hashtable; and then clear it out on LinkProgram.  Additionally, each WebGLProgram should have a generation counter that's incremented whenever the program is re-linked in LinkProgram, and each WebGLUniformLocation should carry along both a program and generation id.

The WebGL test suite needs tests for all of this also, I believe.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

OK, I just have a question. I checked and all our hash table classes seems to internally use 32 bit keys:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.h#80

So, suppose that we do as you describe, with such a 32 bit hash table. The probability of a collision will become more than 50% at (very roughly) 2^16 uniforms (the sqrt of 2^32, see "birthday problem"). Since uniform arrays are handled as individual uniform locations, this means that just by using a uniform array of just 10000 (1e+4) floats, the probability of collision would be significant, and with 1e+5, collision would be prevalent.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

So, I guess my question is, do we have some 64bit hash table available?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No; I would not worry about it at this point -- that's some very serious premature optimization :-)

I would use a nsRefPtrHashtable with a nsUint32HashKey key type.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

OK so anyway I thought a bit more and here's a simple enough solution:

Is it safe to assume that only 1 thread is accessing this data? If yes, why not use a static member uint32 counter in WebGLUniformLocation, that gets incremented in the constructor, and use it to generate unique ids? Then a 32bit hash table using these ids as keys.

Even if there are multiple threads, I'd still be tempted to go this way, the only difference is that we must be atomic when incrementing the counter. Do we have a portable way of doing atomic increments?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

oops, sorry, comment 4 doesn't compile --- need to think more :)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No actually, you might be on to something; everything here is always (and will always be) single threaded.

1) Have a maxUniformLocation member in WebGLProgram -- make it always be the highest uniform location queried + 1.  Have a simple nsTArray of uniform objects, and just keep growing it to contain max entries (leaving uncreated entries remain null).

2) When a program is relinked, clear the array.  Set uniformBase = maxUniformLocation.

3) When a new uniform location is queried, generate an object with location uniformBase + actualLocation.  Set maxUniformLocation to the max of it and uniformBase+actualLocation+1.

4) On uniform access, if the location in the UniformLocation object is < uniformBase, it's invalid.  Otherwise, subtract uniformBase and you have the actual location.

Does that make sense?

The only problem that I see with this is that eventually, you'll run out of room to keep increasing, but given that with 10,000 uniform locations, it would take over 400k linkprograms to do so, that's probably fine.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Wait, I just thought about something: in comment 1 you wrote:

> Easiest way to accomplish both of these would be to store a bunch
> of uniform information on WebGLProgram, potentially including a
> string -> location object hashtable; and then clear it out on LinkProgram.

What if instead of a string -> location_object hashtable, we did a int -> location_object hashtable, using the actual GL uniform locations as the keys? Indeed, they are 32bit, and they are unique!

I had a bit of trouble understanding comment 6: when you write

> 1) Have a maxUniformLocation member in WebGLProgram -- make it always
> be the highest uniform location queried + 1.  Have a simple nsTArray of
> uniform objects, and just keep growing it to contain max entries

By "uniform location" here, do you mean the GL location? I was under the (wrong?) impression that these GL locations could be absolutely 32bit value, not just a nice counter starting from 0..?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

In fact, a good summary of my idea in comment #7 is that the OpenGL implementation already gives us a string-->int "hash function" with no risk of collision at all: that's what glGetUniformLocation is doing :)

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch to preserve equality and reject ulocs from previous generations

Here is a patch...
 - please double check that I got the ref-counted pointers right!
 - WebGLProgram::GetUniformLocationObject() could not be implemented inside of the class body because it requires WebGLUniformLocations to be a complete type. Tell me if you prefer another place than where I put it.
 - I used nsRefPtr's (as opposed to e.g. nsCOMPtr) because that's what nsRefPtrHashTable uses anyway. Hope it's OK.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 449368 [details] [diff] [review]
Patch to preserve equality and reject ulocs from previous generations

Ugh, I hate to r- this given that you checked it in by accident, but...

>+    PRBool NextGeneration()
>+    {
>+        mGeneration++;
>+        mMapUniformLocations.Clear();
>+        return mGeneration != 0;
>+    }

This needs to check for overflow before incrementing, because.. (see below).

>+    nsRefPtr<WebGLUniformLocation> GetUniformLocationObject(GLint glLocation);

Don't return nsRefPtr from methods -- return either already_AddRefed<WebLGUniformLocation> or WebGLUniformLocation*, but I think it should be the former because the object can be created inside the method.  I think this might actually leak UniformLocation objects as written.

>+nsRefPtr<WebGLUniformLocation> WebGLProgram::GetUniformLocationObject(GLint glLocation)
>+{
>+    WebGLUniformLocation *existingLocationObject;
>+    if (mMapUniformLocations.Get(glLocation, &existingLocationObject)) {
>+        return existingLocationObject;
>+    } else {
>+        nsRefPtr<WebGLUniformLocation> loc = new WebGLUniformLocation(mContext, this, glLocation);
>+        mMapUniformLocations.Put(glLocation, loc);
>+        return loc;
>+    }
>+}

See above about this needing to return Already_AddRefed<WebGLUniformLocation>

>     gl->fLinkProgram(progname);
>+    if(!program->NextGeneration())
>+        return NS_ERROR_FAILURE;

This will fail the first time it overflows, but if the user calls LinkProgram() again, the it will succeed because the generation was already incremented (and overflowed) to 0.. so it would be 1 now, and would wrap around.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

This isn't the one that I checked in by accident :)  (that was 569714)

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch to preserve equality and reject ulocs from previous generations, v2

Here's the updated patch. Can you please again check carefully what I'm doing with the refptrs, it's still a bit obscure to me.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 449923 [details] [diff] [review]
Patch to preserve equality and reject ulocs from previous generations, v2

Looks good!

>+    if(!program->NextGeneration())
>+        return NS_ERROR_FAILURE;

        ^ space after if

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/e512b50ff272