Last Comment Bug 750297 - Paris bindings for WebGLUniformLocation
: Paris bindings for WebGLUniformLocation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 750308 760131 762651
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-30 08:54 PDT by Boris Zbarsky [:bz]
Modified: 2012-06-16 06:51 PDT (History)
3 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Switch WebGLUniformLocation to Paris bindings. bjacob (12.89 KB, patch)
2012-06-08 12:31 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
part 1. Add support for objects that do not have a wrapper cache in WebIDL bindings. (25.28 KB, patch)
2012-06-11 23:31 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
part 2. Switch WebGLUniformLocation to Paris bindings. bjacob (12.47 KB, patch)
2012-06-11 23:46 PDT, Boris Zbarsky [:bz]
peterv: review+
jacob.benoit.1: review+
Details | Diff | Review
Interdiff for the change commet 12 asks for (1.63 KB, patch)
2012-06-12 07:30 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
part 1. Add support for objects that do not have a wrapper cache in WebIDL bindings. (25.54 KB, patch)
2012-06-12 07:32 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-04-30 08:54:01 PDT
I ran into a few issues here, which I'll file as bugs blocking this one.
Comment 1 Boris Zbarsky [:bz] 2012-06-08 12:31:32 PDT
Created attachment 631497 [details] [diff] [review]
Switch WebGLUniformLocation to Paris bindings.   bjacob
Comment 2 Boris Zbarsky [:bz] 2012-06-08 12:38:37 PDT
That patch is actually on top of the ones for bug 762651 and bug 760131
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-06-11 11:57:07 PDT
As discussed on #content, my concern with this is that we have seen testcases that create and forget about 20,000 WebGLUniformLocation objects per second (see bug 754303), and so they would probably suffer from having WebGLUniformLocation participate in cycle collection.
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-11 12:19:25 PDT
Do we care about those testcases?  Querying uniform locations should not be in any performance hot path.
Comment 5 Boris Zbarsky [:bz] 2012-06-11 12:26:49 PDT
Current plan:

1)  Make it possible to have new-bindings classes without wrappercache (and hence without
    CC) for classes that can only be created, never gotten.  Not sure what to name this
    annotation yet.  "WrapperCacheLess"?  "wrapperCache"?  This would presumably go into
    the conf file.
2)  Enforce that such classes cannot be wrapped by XPConnect (by having their PreCreate
    always return error).  Then we can only get them out of new bindings.
3)  Annotate WebIDL methods which always return a new object (bikeshed: [Creator]).
4)  Force codegen to fail if a return type of a non-[Creator] method involves these
    classes.

then we can use that setup for WebGLUniformLocation.  We want such a setup for other use cases too (e.g. ImageData), so we might as well do this here and now.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-11 12:45:50 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #4)
> Do we care about those testcases?  Querying uniform locations should not be
> in any performance hot path.

I agree that good code should not be doing that, but we still probably care about these testcases because they are real-world apps from real developers: the testcase in bug 754303 is http://www.ambiera.com/coppercube/demo.php?demo=backyard&mode=webgl
Comment 7 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-11 13:44:30 PDT
I dunno; same question applies -- real-world apps can still be doing things that are horrible for performance, whether with uniform locations, bogus draw calls, or many other things.  Especially given that webgl is a relatively new area, we should focus on documenting best practices, rather than spending a lot of time making bad code run better :)
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-06-11 15:38:59 PDT
This time spent here seems generally useful though:
 - it seems useful to avoid having reference cycles when that's not needed, or conversely, it seems sad to have a reference cycle only because of abstract reasons coming from the bindings system;
 - it seems plausible that there might be more object types in the same case (outside of WebGL) and that Boris' plan in comment 5 would apply to them too.
Comment 9 Boris Zbarsky [:bz] 2012-06-11 17:58:51 PDT
> it seems plausible that there might be more object types in the same case 

It's not just plausible.  It's known.  There are objects for which we want the comment 5 setup for basic sanity, not just for performance.
Comment 10 Boris Zbarsky [:bz] 2012-06-11 23:31:24 PDT
Created attachment 632145 [details] [diff] [review]
part 1.  Add support for objects that do not have a wrapper cache in WebIDL bindings.
Comment 11 Boris Zbarsky [:bz] 2012-06-11 23:46:09 PDT
Created attachment 632146 [details] [diff] [review]
part 2.  Switch WebGLUniformLocation to Paris bindings.   bjacob
Comment 12 Peter Van der Beken [:peterv] 2012-06-12 05:21:47 PDT
Comment on attachment 632145 [details] [diff] [review]
part 1.  Add support for objects that do not have a wrapper cache in WebIDL bindings.

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

::: dom/bindings/BindingUtils.h
@@ +351,5 @@
> +
> +  // We wrapped in the compartment of scope, so we better be in the
> +  // compartment of cx already.
> +  MOZ_ASSERT(js::IsObjectInContextCompartment(scope, cx));
> +  MOZ_ASSERT(js::IsObjectInContextCompartment(obj, cx));

I'm not sure this is what we want. Wouldn't it make more sense to create in the compartment of the this object of the caller of WrapNewBindingNonWrapperCachedObject?
Comment 13 Boris Zbarsky [:bz] 2012-06-12 06:31:38 PDT
Well, that's what I'm sort of doing... except the this object might be a security wrapper.  Do you want me to basically unwrap security wrappers on the scope and enter the compartment of the result before creating obj?
Comment 14 Boris Zbarsky [:bz] 2012-06-12 07:30:36 PDT
Created attachment 632243 [details] [diff] [review]
Interdiff for the change commet 12 asks for
Comment 15 Boris Zbarsky [:bz] 2012-06-12 07:32:08 PDT
Created attachment 632245 [details] [diff] [review]
part 1.  Add support for objects that do not have a wrapper cache in WebIDL bindings.
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-06-14 12:54:28 PDT
Comment on attachment 632146 [details] [diff] [review]
part 2.  Switch WebGLUniformLocation to Paris bindings.   bjacob

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

r=me on the 5% of this patch that I can review; wait for peterv for the remaining 95%.

Some questions:

::: dom/base/nsDOMClassInfo.h
@@ +486,5 @@
>      return new WebGLExtensionSH(aData);
>    }
>  };
>  
> +// WebGLUniformLocation scriptable helper

I would like to understand: if every class that gets ported to the new DOM bindings requires adding such 20 lines to nsDOMClassInfo.h, then this file is going to grow really big, no?

Could we have it so that each class can declare and implement its own ScriptableHelper in its own file/directory so that it stays "local"?

::: dom/base/nsDOMClassInfoID.h
@@ -59,5 @@
> -DOMCI_CASTABLE_INTERFACE(nsIWebGLUniformLocation,                             \
> -                         nsIWebGLUniformLocation, 11, _extra)                 \
> -DOMCI_CASTABLE_INTERFACE(nsIDOMImageData, nsIDOMImageData, 12, _extra)        \
> -DOMCI_CASTABLE_NAMESPACED_INTERFACE(mozilla, WebGLUniformLocation,            \
> -                                    nsIWebGLUniformLocation, 13, _extra)

Can you explain why this is not needed anymore?

Also, this is now jumping straight from 9 to 12. Does that matter?
Comment 17 Boris Zbarsky [:bz] 2012-06-14 18:13:02 PDT
> if every class that gets ported to the new DOM bindings requires adding such 20 lines to
> nsDOMClassInfo.h, then this file is going to grow really big, no?

The addition here was only because I was using the rare-case wrapper-cache-less bindings and wanted to make sure that the class could not be wrapped by XPConnect by accident.  For bindings that uses a wrapper cache this won't be necessary.  It also wouldn't be necessary if the object didn't inherit from nsISupports, though that might take a bit more work...

I agree the transitional period is kinda sucky, though.

Note that realistically we could probably rename this helper class (e.g. to DontWrapMeSH) and use it for _all_ the classinfo for new-binding objects without wrapper caches.  Maybe I should have given it a name along those lines, in fact....  Peter, thoughts?

> Can you explain why this is not needed anymore?

It was needed before to make argument unwrapping for the WebGLContext quickstubs fast.  And then the new bindings used the same infrastructure for arguments that were not themselves new-binding objects.  But at this point WebGLContext doesn't use quickstubs  and WebGLUniformLocation has a new-binding wrapper, so it no longer needs the castable native machinery for fast unwrapping.  And in fact, it _can't_ use that machinery anymore; there's a comment about that in the file (added by some patches Peter has not landed yet) that just wasn't close enough to be in the context.

> Also, this is now jumping straight from 9 to 12. Does that matter?

Nope.  They're just shifts for bitflags; it doesn't matter if a few are unused, and we're in the process of getting rid of them anyway as we switch things to the new bindings.  Peter has a patch to nix the ImageData one, for example.
Comment 18 Peter Van der Beken [:peterv] 2012-06-15 03:03:13 PDT
Comment on attachment 632146 [details] [diff] [review]
part 2.  Switch WebGLUniformLocation to Paris bindings.   bjacob

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

I used nsNewDOMBindingNoWrapperCache for ImageData.

::: dom/base/nsWrapperCache.h
@@ +249,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                         \
>    NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
>  
> +#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(_class, _field1,\
> +                                                _field2)        \

You don't seem to be using this?
Comment 19 Boris Zbarsky [:bz] 2012-06-15 08:00:22 PDT
> I used nsNewDOMBindingNoWrapperCache for ImageData.

I'll rename to that here, yeah.  nsNewDOMBindingNoWrapperCacheSH, more precisely.

> You don't seem to be using this?

Indeed, no longer.  I still think it's worth adding.

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