Boris Zbarsky [:bzbarsky] Assignee
	Description • 12 years ago

I ran into a few issues here, which I'll file as bugs blocking this one.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 12 years ago

That patch is actually on top of the ones for bug 762651 and bug 760131

	Benoit Jacob [:bjacob] (mostly away)
	Comment 3 • 12 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 4 • 12 years ago

Do we care about those testcases?  Querying uniform locations should not be in any performance hot path.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 6 • 12 years ago

(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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 7 • 12 years ago

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 :)

	Benoit Jacob [:bjacob] (mostly away)
	Comment 8 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

> 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.

	Peter Van der Beken [:peterv]
	Comment 12 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

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?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 16 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 12 years ago

> 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.

	Peter Van der Beken [:peterv]
	Comment 18 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 12 years ago

> 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/338df6399528
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce1e213f9a3

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/338df6399528
https://hg.mozilla.org/mozilla-central/rev/7ce1e213f9a3