Paris bindings for WebGLUniformLocation

RESOLVED FIXED in mozilla16

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug)

unspecified
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

I ran into a few issues here, which I'll file as bugs blocking this one.
Depends on: 750308
Depends on: 750309
No longer depends on: 750309
Created attachment 631497 [details] [diff] [review]
Switch WebGLUniformLocation to Paris bindings.   bjacob
Attachment #631497 - Flags: review?(peterv)
Attachment #631497 - Flags: review?(bjacob)
Whiteboard: [need review]
That patch is actually on top of the ones for bug 762651 and bug 760131
Depends on: 762651, 760131
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.
Attachment #631497 - Flags: review?(bjacob)
Do we care about those testcases?  Querying uniform locations should not be in any performance hot path.
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.
(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
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 :)
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.
> 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.
Created attachment 632145 [details] [diff] [review]
part 1.  Add support for objects that do not have a wrapper cache in WebIDL bindings.
Attachment #632145 - Flags: review?(peterv)
Created attachment 632146 [details] [diff] [review]
part 2.  Switch WebGLUniformLocation to Paris bindings.   bjacob
Attachment #632146 - Flags: review?(peterv)
Attachment #632146 - Flags: review?(bjacob)
Attachment #631497 - Attachment is obsolete: true
Attachment #631497 - Flags: review?(peterv)
Summary: Paris bindings for WebGLUniformLocation (and maybe other WebGLBoundObject types) → Paris bindings for WebGLUniformLocation
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?
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?
Created attachment 632243 [details] [diff] [review]
Interdiff for the change commet 12 asks for
Created attachment 632245 [details] [diff] [review]
part 1.  Add support for objects that do not have a wrapper cache in WebIDL bindings.
Attachment #632245 - Flags: review?(peterv)
Attachment #632145 - Attachment is obsolete: true
Attachment #632145 - Flags: review?(peterv)
Attachment #632245 - Flags: review?(peterv) → review+
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?
Attachment #632146 - Flags: review?(bjacob) → review+
> 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 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?
Attachment #632146 - Flags: review?(peterv) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/338df6399528
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce1e213f9a3
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/338df6399528
https://hg.mozilla.org/mozilla-central/rev/7ce1e213f9a3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.