Last Comment Bug 648801 - Prototype a proxy-based NodeList implementation
: Prototype a proxy-based NodeList implementation
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
http://hg.mozilla.org/users/bzbarsky_...
Depends on: 649887 691475 678528 684418 691472 691499 691706 691707 693258 693323 693364 693563 693811 693894 694009 694308 694594 695867 697643 698549 698551 699826 715156 717842 724033 734002 759621 794990
Blocks: ParisBindings 300519 622298 665894 686120
  Show dependency treegraph
 
Reported: 2011-04-09 17:53 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-09-28 16:08 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.20 KB, patch)
2011-04-09 17:56 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (17.23 KB, patch)
2011-04-09 17:56 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Patch on top of the previous one to make it sorta compile and run (4.41 KB, patch)
2011-04-10 02:17 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Patch on top of _that_ to not create a new wrapper every time (1.30 KB, patch)
2011-04-10 02:43 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
And now without the crashing (1.30 KB, patch)
2011-04-10 02:54 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Without crashing for real (1.59 KB, patch)
2011-04-10 02:56 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Rollup patch (22.06 KB, patch)
2011-04-10 21:58 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Patch as of this morning (21.29 KB, patch)
2011-04-12 13:56 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
patch (33.70 KB, patch)
2011-04-12 14:46 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (22.50 KB, patch)
2011-04-12 15:12 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch with item caching (untested) (32.25 KB, patch)
2011-04-13 10:50 PDT, Andreas Gal :gal
bzbarsky: feedback-
Details | Diff | Splinter Review
Some simple performance tests (31.83 KB, application/zip)
2011-08-01 12:05 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
JSAPI changes v1 (5.77 KB, patch)
2011-08-15 05:34 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
JSAPI changes v1.1 (14.58 KB, patch)
2011-08-15 06:51 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Main patch v1 (252.00 KB, patch)
2011-08-15 11:22 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Main patch v1.1 (256.51 KB, patch)
2011-08-20 11:40 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Main patch v1.2 (256.87 KB, patch)
2011-08-22 09:08 PDT, Peter Van der Beken [:peterv]
jst: review+
mrbkap: review+
bzbarsky: review+
Details | Diff | Splinter Review
Generated code example (dombindings_gen.cpp) (15.58 KB, text/plain)
2011-09-16 09:35 PDT, Peter Van der Beken [:peterv]
no flags Details
Generated code example (dombindings_gen.h) (1.74 KB, text/plain)
2011-09-16 09:37 PDT, Peter Van der Beken [:peterv]
no flags Details

Description Boris Zbarsky [:bz] (still a bit busy) 2011-04-09 17:53:00 PDT
For great speed.
Comment 1 Andreas Gal :gal 2011-04-09 17:56:03 PDT
Created attachment 524898 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2011-04-09 17:56:59 PDT
Created attachment 524899 [details] [diff] [review]
patch
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-04-10 02:17:29 PDT
Created attachment 524930 [details] [diff] [review]
Patch on top of the previous one to make it sorta compile and run

Still no support for item(), so the browser UI is a bit broken.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-04-10 02:43:04 PDT
Created attachment 524931 [details] [diff] [review]
Patch on top of _that_ to not create a new wrapper every time

OK, so with this I see a 262 on my dromaeo firstChild test (instead of 200 with our old impl).  But on my synthetic tests we're a bit slower than we used to be on one of them, and crash on the other one.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-04-10 02:54:59 PDT
Created attachment 524933 [details] [diff] [review]
And now without the crashing

This at least runs.

Looking at the timings and profiles, it seems that [n] has gotten a good bit faster and that .length is _really_ slow now.  So we should fix that second bit, and then measure some more.

The xpconnect code we added is also slower than it should be, but we can worry about that later. If I try to take the normal fast paths we call JS_WrapObject on our nodelist proxy and that crashes...
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-04-10 02:56:41 PDT
Created attachment 524934 [details] [diff] [review]
Without crashing for real
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-04-10 21:58:45 PDT
Created attachment 525034 [details] [diff] [review]
Rollup patch

This is as of this afternoon.  When combined with the patch from bug 648943, this is about 2x faster than the old code.

Things that still need to be done:

1)  Correctly hook up the wrapping process.  Right now it's hooked into the wrong place, doesn't use the right prototype object, etc.

2)  Implement .item() and probably .namedItem(); need to figure out what the plan is for the latter.

3)  Implement expandos.

4)  Fix cycle collector integration.

5)  Make sure that security-wrapping works correctly.

Plus whatever fixmes are left in the code once we do all of that.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-04-11 04:35:43 PDT
(In reply to comment #7)
> .namedItem()

I'd prefer not to add support for that. In any case, I hope there aren't any plans to change observable behaviour in this bug (except for speed).
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-04-11 08:16:37 PDT
> I'd prefer not to add support for that.

Uh... it's required by the spec and web compat.  Keep in mind that this bug is about _all_ our nodelist/htmlcollection implemenations.  And the latter needs .namedItem.

> I hope there aren't any plans to change observable behaviour

There are, actually.  The current observable behavior that [n] keeps returning the node that used to be at that index even if the list length is now <= n will go away.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-04-11 12:23:52 PDT
Per our discussion today, the plan goes like this:

* Add a virtual method on nsWrapperCache to decide what sort of object (proxy or
  not, jsclass, proxy handler, etc) to create.  Move towards having
  nsWrapperCache on all DOM objects and renaming it to dom::Object or domObject
  or nsISupportsDOMObject or whatever.  This partially addresses item 1 in
  comment 7
* Use a separate object for expandos.  Have the nodelist c++ object mark the 
  expando object (need a separate map for this, with CC hooks); have the proxy
  own the nodelist.
* Implement security wrappers for the proxy case by just creating a new proxy
  that has a handler that knows to not look at the expandos, etc.
* Implement prototypes for security wrappers by basically creating a security
  wrapper around the DOM prototype; the proxy will ignore the prototype in this
  case anyway, so all we need this for is instanceof.
* For the non-proxy case, we can think about how to best do things, but having
  some sort of extended jsclass per C++ class which knows about which things
  the C++ class can cast to and how is probably the way to go.
* We'll need explicit hooks for returning non-DOM objects from DOM ones in the
  rare cases this happens in.
Comment 11 Peter Van der Beken [:peterv] 2011-04-11 18:09:39 PDT
(In reply to comment #10)
> * Add a virtual method on nsWrapperCache to decide what sort of object (proxy
> or
>   not, jsclass, proxy handler, etc) to create.

We might want to add a member instead (since nsWrapperCache doesn't inherit from nsISupports, so we'd add another vtable pointer).
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-04-12 13:56:23 PDT
Created attachment 525504 [details] [diff] [review]
Patch as of this morning
Comment 13 Andreas Gal :gal 2011-04-12 14:46:51 PDT
Created attachment 525525 [details] [diff] [review]
patch
Comment 14 Andreas Gal :gal 2011-04-12 15:12:06 PDT
Created attachment 525532 [details] [diff] [review]
patch
Comment 15 Andreas Gal :gal 2011-04-13 10:50:17 PDT
Created attachment 525725 [details] [diff] [review]
patch with item caching (untested)
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 13:22:43 PDT
Based on timing measurements and profiles so far that cache is not working.  Rebuilding debug to figure out what's going on.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 13:38:25 PDT
This also doesn't compile if I do a whole-tree build; I'll try to fix that later tonight.  By the way, I have an interdiff of this now, no thanks to bugzilla, but _please_ can we post interdiffs and not whole diffs against m-c?  At some point this will need to be reviewed and we will need to break it up into sane chunks; doing that is much easier if we have it in pieces already.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-04-13 21:17:54 PDT
We now have that compiling; item caching helps some, but ICs are needed to really win there (bug 649887).

I pushed a more or less rationalized queue of work so far to http://hg.mozilla.org/users/bzbarsky_mozilla.com/nodelist-bindings
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-08-01 12:05:14 PDT
Created attachment 549881 [details]
Some simple performance tests
Comment 20 Peter Van der Beken [:peterv] 2011-08-15 05:34:30 PDT
Created attachment 553152 [details] [diff] [review]
JSAPI changes v1

These are the JSAPI changes that we need for the other patches.
Comment 21 Peter Van der Beken [:peterv] 2011-08-15 06:51:15 PDT
Created attachment 553163 [details] [diff] [review]
JSAPI changes v1.1
Comment 22 Peter Van der Beken [:peterv] 2011-08-15 11:22:23 PDT
Created attachment 553225 [details] [diff] [review]
Main patch v1
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2011-08-15 13:24:16 PDT
Your python files need license headers, and in nsWrapperCacheInlines.h, the Initial Developer is MoFo, not MoCo.
Comment 24 Peter Van der Beken [:peterv] 2011-08-20 11:40:22 PDT
Created attachment 554653 [details] [diff] [review]
Main patch v1.1

Some minor fixes.
Comment 25 Peter Van der Beken [:peterv] 2011-08-22 09:08:15 PDT
Created attachment 554877 [details] [diff] [review]
Main patch v1.2

Merged to trunk and simplified wrapper cache a little bit.
Comment 26 Peter Van der Beken [:peterv] 2011-09-16 09:35:46 PDT
Created attachment 560560 [details]
Generated code example (dombindings_gen.cpp)
Comment 27 Peter Van der Beken [:peterv] 2011-09-16 09:37:23 PDT
Created attachment 560561 [details]
Generated code example (dombindings_gen.h)
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-09-16 14:57:05 PDT
Comment on attachment 554877 [details] [diff] [review]
Main patch v1.2

>+++ b/dom/base/nsWrapperCache.h
>+  JSObject* GetExpandoObject() const;

Please document this.

>+  virtual JSObject* WrapObject(JSContext *cx, XPCWrappedNativeScope *scope,
>+                               bool *wrapped)

"wrapped" should probably be called "triedToWrap".

And document that the out param is meaningless if non-null is returned.

>+++ b/dom/base/nsWrapperCacheInlines.h
>+    reinterpret_cast<JSObject*>(mWrapperPtrBits & ~kWrapperBitMask);

The "mWrapperPtrBits & ~kWrapperBitMask" pattern appears in a bunch of places here.  Can we give it a nice name?

>+nsWrapperCache::RemoveExpandoObject()

We need to document somewhere 

>+    if (mozilla::dom::binding::isExpandoObject(obj)) {

Why do we not need to RemoveDOMExpandoObject in the case when |obj| is a proxy?

Documenting the general ownership interaction between the nsWrapperCache, the proxy, and the expando object would be good.

>+++ b/dom/interfaces/html/nsIDOMHTMLOptionsCollection.idl
>+  // FIXME item should just be inherited from nsIDOMHTMLCollection

File a followup bug on this and remove the stuff that doesn't need to be here from this interface?

>+++ b/dom/tests/mochitest/bugs/test_bug633133.html
>-ok(!("" in divCollection), "empty string shouldn't be in the div collection");
>+ok("" in divCollection, "empty string should be in the div collection");

I think the old behavior was correct.  Can we have a followup bug on changing it back, and in particular on using js_CheckForStringIndex instead of the GetArrayIndexFromId thing we're doing right now?

>+++ b/js/src/xpconnect/src/codegen.py

Please file a followup to share this code better with quickstubs, ok?

Also, could you attach the generated files that actually get produced in this case?

>+++ b/js/src/xpconnect/src/dombindings.cpp
>+ListBase<LC>::instanceIsListObject(JSContext *cx, JSObject *obj)
>+        // FIXME: Throw a proper DOM exception.

Please file a followup.

>+ListBase<LC>::namedItem(JSContext *cx, JSObject *obj, jsval *name, NameGetterType &result,

This should assert that |obj| is the right sort of object.

>+ListBase<LC>::getPrototype(JSContext *cx, XPCWrappedNativeScope *scope)
>+        JSFunction *fun = JS_NewFunctionById(cx, sProtoMethods[n].native, 1, 0,

This is assuming all the methods take one argument.  I think that's true for now, but maybe a followup for getting the arg count from sProtoMethods[n]?

>+    if (!JS_DefinePropertyById(cx, interfacePrototype, s_constructor_id,
>+                               OBJECT_TO_JSVAL(interface), nsnull, nsnull, JSPROP_SHARED))

This looks wrong; having a value is not compatible with JSPROP_SHARED.  What should the flags here actually be?

>+ListBase<LC>::create(JSContext *cx, XPCWrappedNativeScope *scope, ListType *aList,
>+    if (parent != scope->GetGlobalJSObject()) {

This will always test false; we should be looking at the global of parent on the LHS.

>+CheckForStringIndex(jsid id)

Please file a followup to make js_CheckForStringIndex have this inlining optimization.

>+JSClass ExpandoClass = {
>+    "DOM Expando object",

Maybe "DOM proxy binding expando object"

>+ListBase<LC>::defineProperty(JSContext *cx, JSObject *proxy, jsid id, PropertyDescriptor *desc)
>+        if (JSID_IS_ATOM(id))
>+            id = CheckForStringIndex(id);

You don't need the JSID_IS_ATOM check here.

>+ListBase<LC>::getOwnPropertyNames(JSContext *cx, JSObject *proxy, AutoIdVector &props)
>+    // FIXME: Add named items

Followup, please.

>+ListBase<LC>::delete_(JSContext *cx, JSObject *proxy, jsid id, bool *bp)

Do you need to check whether |proxy| is an XrayWrapper here?

>+ListBase<LC>::enumerate(JSContext *cx, JSObject *proxy, AutoIdVector &props)
>+    // FIXME: enumerate proto as well

Again, followuo?

>+ListBase<LC>::resolveNativeName(JSContext *cx, JSObject *proxy, jsid id, PropertyDescriptor *desc)

Please assert that |proxy| is an XrayWrapper here.  Also, why is the JS_NewFunctionById getting passed proxy->getParent() while we use proto->getParent() when setting up the proto normally?

Also, same comment here about "1" as when setting up the prototype.

>+ListBase<LC>::nativeGet(JSContext *cx, JSObject *proxy, JSObject *proto, jsid id, bool *found, Value *vp)
>+            if (!sProtoProperties[n].getter)
>+                return false;

This can cause a return false without throwing an exception on some codepaths (e.g. via ::get()).  Please audit the callers and fix as needed.

>+            *vp = proto->getSlot(n);

Can we assert somewhere earlier in this method that the right stuff is true to make this safe?

>+ListBase<LC>::getPropertyOnPrototype(JSContext *cx, JSObject *proxy, jsid id, bool *found,
>+    *found = !vp->isUndefined();

This doesn't seem right if an expando on the proto has the value |undefined|.  We probably need to be more careful here.

>+ListBase<LC>::get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, Value *vp)
>+        if (!vp->isUndefined())
>+            return true;

Same issue here as in getPropertyOnPrototype.

>+NoBase::getPrototype(JSContext *cx, XPCWrappedNativeScope *scope)
>+    // will look up a prototype on the global by using the class' name an we'll recurse into

s/an/and/

>+++ b/js/src/xpconnect/src/nsXPConnect.cpp
>+        // FIXME: Provide a fast non-refcounting way to get the canonical
>+        //        nsISupports from the proxy.

Please file a followup.

>+++ b/js/src/xpconnect/src/xpcconvert.cpp
>+            if(flat) {
>+                if(!JS_WrapObject(ccx, &flat))

|flat| can only be null if the new bindings are preffed off, right?  Please add a comment here about that, and maybe file a bug to resimplify this code once we remove that pref.

>+++ b/js/src/xpconnect/src/xpcjsruntime.cpp
>         {
>+            JSAutoRequest ar(cx);

Document that the scope there is for the JSAutoRequest so it goes out of scope before the DefineStaticJSVals call?

>+++ b/js/src/xpconnect/src/xpcprivate.h
>+    static XPCWrappedNativeScope *GetNativeScope(JSContext *cx, JSObject *obj)
>+    {
>+        js::Value v = obj->getSlot(JSCLASS_GLOBAL_SLOT_COUNT);

There should presumably be an assert somewhere here about |obj| being an XPConnect global or something?

>+++ b/xpcom/idl-parser/xpidl.py
>+            elif name == 'setter':
>+                if (len(self.params) != 2):
>+                    raise IDLError("Methods marked as getter must take 2 arguments", aloc)

s/getter/setter/

r=me with the above issues fixed.
Comment 29 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2011-09-21 14:33:17 PDT
Comment on attachment 554877 [details] [diff] [review]
Main patch v1.2

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

Peter and I only went over the Xray stuff.

::: js/src/xpconnect/src/dombindings.cpp
@@ +112,5 @@
> +
> +    XPCLazyCallContext lccx(JS_CALLER, cx, scope);
> +
> +    nsresult rv;
> +    if(!XPCConvert::NativeInterface2JSObject(lccx, rval, NULL, helper, NULL, NULL,

Please "harmonize" this style with the rest of the code around here.

@@ +274,5 @@
> +bool
> +ListBase<LC>::instanceIsListObject(JSContext *cx, JSObject *obj)
> +{
> +    if (XPCWrapper::IsSecurityWrapper(obj)) {
> +        obj = XPCWrapper::Unwrap(cx, obj);

Per our discucssion, if this is a security wrapper, we should instead verify that obj is from the same scope as the getter being called and if so should call obj->unwrap() (thus skipping the security check that XPCWrapper::Unwrap does).

If a caller doesn't have a callee to pass in, then we can fall back onto using XPCWrapper::Unwrap.

::: js/src/xpconnect/wrappers/XrayWrapper.cpp
@@ +1003,5 @@
> +
> +    if (!JS_GetPropertyDescriptorById(cx, holder, id, JSRESOLVE_QUALIFIED, jspropdesc))
> +        return false;
> +    if (desc->obj) {
> +        desc->obj = obj;

These should use wrapper instead of obj.

@@ +1007,5 @@
> +        desc->obj = obj;
> +        return true;
> +    }
> +
> +    // FIXME Check for recursion?

We don't have to worry about recursion here because we're not calling into a scriptable helper that can do random things, we're only calling into the proxy-implemented hook that we control.
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-28 13:19:08 PDT
Comment on attachment 554877 [details] [diff] [review]
Main patch v1.2

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

r=jst with these comments and others comments addressed.

::: dom/base/nsDOMClassInfo.h
@@ +204,4 @@
>    static PRInt32 GetArrayIndexFromId(JSContext *cx, jsid id,
>                                       PRBool *aIsNumber = nsnull);
>  
> +protected:

Maybe just move this method into an already public section to avoid adding public/protected here?

::: dom/base/nsWrapperCache.h
@@ +119,5 @@
> +   * is returned then wrapped indicates whether an error occurred, if it's false
> +   * then the object doesn't actually support creating a wrapper through its
> +   * WrapObject hook.
> +   */
> +  virtual JSObject* WrapObject(JSContext *cx, XPCWrappedNativeScope *scope,

Given that this is adding a virtual function here we should bump the IID of every interface that inherits this, especially commonly used ones like nsINode and nsIDocument etc.

::: js/src/xpconnect/src/nsXPConnect.cpp
@@ +1539,5 @@
> +        return (nsISupports*)xpc_GetJSPrivate(obj2);
> +
> +    if(mozilla::dom::binding::instanceIsProxy(aJSObj)) {
> +        // FIXME: Provide a fast non-refcounting way to get the canonical
> +        //        nsISupports from the proxy.

Bug on file for this?

::: js/src/xpconnect/src/xpcconvert.cpp
@@ +1215,3 @@
>  
> +                return CreateHolderIfNeeded(ccx, flat, d, dest);
> +            }

Don't we want to throw here if !flat? I.e. if ConstructProxyObject() fails, isn't that an error case where we want to return rather than proceed and try to wrap etc...?

::: xpcom/idl-parser/xpidl.py
@@ +861,5 @@
> +                    raise IDLError("Methods marked as getter must take 1 argument", aloc)
> +                self.getter = True
> +            elif name == 'setter':
> +                if (len(self.params) != 2):
> +                    raise IDLError("Methods marked as getter must take 2 arguments", aloc)

s/getter/setter/ in the error message.
Comment 31 Peter Van der Beken [:peterv] 2011-10-10 07:35:45 PDT
This landed, but it was a bit bumpy.

https://hg.mozilla.org/mozilla-central/rev/39e41a138c0b
https://hg.mozilla.org/mozilla-central/rev/bd16e7107228
https://hg.mozilla.org/mozilla-central/rev/aca2001154a8
https://hg.mozilla.org/mozilla-central/rev/e76de73e29bf
https://hg.mozilla.org/mozilla-central/rev/8dbb002f6dc6
https://hg.mozilla.org/mozilla-central/rev/3a151ac8a748
https://hg.mozilla.org/mozilla-central/rev/0b6fe35629ae
https://hg.mozilla.org/mozilla-central/rev/0a4641db636d
https://hg.mozilla.org/mozilla-central/rev/73afd09ad56a
https://hg.mozilla.org/mozilla-central/rev/bf437c634fda
https://hg.mozilla.org/mozilla-central/rev/dc150e59693a
https://hg.mozilla.org/mozilla-central/rev/71625e542826
https://hg.mozilla.org/mozilla-central/rev/3745e14d4407
https://hg.mozilla.org/mozilla-central/rev/f2e77f10570e
https://hg.mozilla.org/mozilla-central/rev/41fdf93335d8
https://hg.mozilla.org/mozilla-central/rev/6bfceb865498
https://hg.mozilla.org/mozilla-central/rev/93037e2151f3
https://hg.mozilla.org/mozilla-central/rev/d64ee7195476
https://hg.mozilla.org/mozilla-central/rev/1169117ea7f1
https://hg.mozilla.org/mozilla-central/rev/2db768787e7b
https://hg.mozilla.org/mozilla-central/rev/799c6ff3f36e
https://hg.mozilla.org/mozilla-central/rev/b8c260359f92
https://hg.mozilla.org/mozilla-central/rev/3630c0108909
https://hg.mozilla.org/mozilla-central/rev/c6c73791e69e
https://hg.mozilla.org/mozilla-central/rev/b336d3ca7d39
https://hg.mozilla.org/mozilla-central/rev/4742146da66f
https://hg.mozilla.org/mozilla-central/rev/f1d2301aec7c
https://hg.mozilla.org/mozilla-central/rev/b88815f00df9
https://hg.mozilla.org/mozilla-central/rev/9c65c03b412a
https://hg.mozilla.org/mozilla-central/rev/ea78bc0b06ff
https://hg.mozilla.org/mozilla-central/rev/53cb5c81ba84
https://hg.mozilla.org/mozilla-central/rev/db88d26a4cdd
https://hg.mozilla.org/mozilla-central/rev/dce1d5524739
https://hg.mozilla.org/mozilla-central/rev/adb562fa3328
https://hg.mozilla.org/mozilla-central/rev/87df4f372ec0
https://hg.mozilla.org/mozilla-central/rev/8b2e402a54b7
https://hg.mozilla.org/mozilla-central/rev/a81ccdc0be58
https://hg.mozilla.org/mozilla-central/rev/0679aba1d739
https://hg.mozilla.org/mozilla-central/rev/7b6a905eaecc
https://hg.mozilla.org/mozilla-central/rev/6909fd27d2b7
https://hg.mozilla.org/mozilla-central/rev/abd2ed17eaa1
https://hg.mozilla.org/mozilla-central/rev/669ec5b8b282
https://hg.mozilla.org/mozilla-central/rev/7b0e7af95fcb
https://hg.mozilla.org/mozilla-central/rev/5d583adcbde6
https://hg.mozilla.org/mozilla-central/rev/09d029625faf
https://hg.mozilla.org/mozilla-central/rev/432f3a96bc2b

and a bustage fix (removing an assertion that I planned to remove anyway in the patch in bug 693301):

https://hg.mozilla.org/mozilla-central/rev/6751379365e3

The final checkin turns on the bindings:

https://hg.mozilla.org/mozilla-central/rev/aee2bf4eb5f8

Ideally we'd be able to back out just that last one if there are serious regressions, but we need to fix bug 693258 first, since the current code has a bunch of orange tests if we turn off the bindings.
Comment 32 :Ehsan Akhgari 2011-10-10 09:27:21 PDT
This broke clang builds, see bug 693323.
Comment 33 :Ms2ger (⌚ UTC+1/+2) 2011-10-10 10:30:52 PDT
https://hg.mozilla.org/mozilla-central/rev/b336d3ca7d39

    1.36 +    if (!XPCConvert::NativeInterface2JSObject(lccx, rval, NULL, helper, NULL, NULL,
    1.37 +                                              allowNativeWrapper, OBJ_IS_NOT_GLOBAL, &rv)) {
    1.38 +        // I can't tell if NativeInterface2JSObject throws JS exceptions
    1.39 +        // or not.  This is a sloppy stab at the right semantics; the
    1.40 +        // method really ought to be fixed to behave consistently.


Did you file that?

    2.18 +// nsGlobalWindow implements nsWrapperCache, but doesn't always use it. Don't
    2.19 +// try to use it without fixing that first.

And that?
Comment 34 Peter Van der Beken [:peterv] 2011-10-10 15:00:55 PDT
(In reply to Ms2ger from comment #33)
>     1.36 +    if (!XPCConvert::NativeInterface2JSObject(lccx, rval, NULL,
> helper, NULL, NULL,
>     1.37 +                                              allowNativeWrapper,
> OBJ_IS_NOT_GLOBAL, &rv)) {
>     1.38 +        // I can't tell if NativeInterface2JSObject throws JS
> exceptions
>     1.39 +        // or not.  This is a sloppy stab at the right semantics;
> the
>     1.40 +        // method really ought to be fixed to behave consistently.
> 
> 
> Did you file that?

Copied from xpc_qsXPCOMObjectToJsval. I guess I can file, but I didn't write this code.

>     2.18 +// nsGlobalWindow implements nsWrapperCache, but doesn't always
> use it. Don't
>     2.19 +// try to use it without fixing that first.
> 
> And that?

What about it?
Comment 35 Octoploid 2011-10-11 05:12:14 PDT
This breaks profiledbuild on Linux x86_64. Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=693563
Comment 36 Mihaela Velimiroviciu (:mihaelav) 2012-01-09 07:37:49 PST
Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0

I have run the attached performance tests (from comment #19) a few times and I have got the following results:

Win7:
-dromaeo-firstChild.html: 115.53, 125.24, 126, 127.49, 125.37, 126, 127
-test1.html: 1294-732, 1269-825, 1321-733, 1265-707, 1234-736
-test2.html: 1361-1032, 1613-1006, 1647-1006, 1632-1010, 1592-1017

Mac 10.6:
-dromaeo-firstChild.html: 108.134, 114.087, 113.207, 112.662, 114.427
-test1.html: 1641-856, 1602-844, 1612-855, 1587-851, 1646-876
-test2.html: 2181-1319, 2092-1335, 2159-1358, 2081-1360, 2135-1314

Linux 11.10:
-dromaeo-firstChild.html: 56, 112.214, 125.784, 125.748, 125.373, 126.623
-test1.html: 1481-890, 1450-880, 1455-894, 1473-879, 1459-880
-test2.html: 1888-1173, 1780-1143, 1812-1141, 1781-1143, 1830-1141

For the latest released Firefox version (9.0.1) the results are:
Mozilla/5.0 (Windows NT 6.1; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Mozilla/5.0 (X11; Linux i686; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0.1) Gecko/20100101 Firefox/9.0.1

Win7:
-dromaeo-firstChild.html: 86.395, 92.721, 94.339, 94.059, 86.395
-test1.html: 1521-828, 1505-883, 1465-836, 1503-831, 1469-805
-test2.html: 1258-681, 1253-662, 1240-639, 1258-630,1273-673

Mac 10.6:
-dromaeo-firstChild.html: 80.838, 83.168, 83.168, 83.416, 83.416
-test1.html: 1687-1041, 1654-997, 1700-1005, 1641-1019, 1699-996
-test2.html:1493-827, 1459-828, 1527-821, 1489-812, 1526-816

Linux 11.10:
-dromaeo-firstChild.html: 104.477, 106.045, 93.311, 106.256, 106.256
-test1.html: 1572-809, 1411-816, 1402-819, 1405-811, 1428-811
-test2.html: 1532-757, 1398-754, 1395-825, 1384-772, 1388-764


Are there any specs for the results? What ranges should they be? I see that some results are lower on the older version (9.0.1) and some are lower on the new one (10.0). Is this expected?
Also, is there anything else to do to verify this feature (DOM Bindings: Node List and Array Bindings)?

Thank you!
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2012-01-09 07:51:27 PST
> Is this expected?

For dromaeo-firstChild.html, the number is runs/s. So bigger is better.

For the other two, the number is ms.  So smaller is better.

There are no obvious specs for the ranges.
Comment 38 Mihaela Velimiroviciu (:mihaelav) 2012-01-10 07:12:42 PST
Will someone continue to work to improve the results that are worse on Firefox 10 than Firefox 9?
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2012-01-10 08:30:01 PST
"Probably".  Which ones are those?
Comment 40 Mihaela Velimiroviciu (:mihaelav) 2012-01-11 00:02:55 PST
From the results I got, times for test1.html on Linux and for test2.html on all platforms are greater on Firefox 10 than on Firefox 9 and as far as I understood from comment #37 it should be the other way around. Isn't this what the bug is about (improve performance/speed)?
Please let me know if there is anything else to do for QA.
Thank you!
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2012-01-11 06:57:06 PST
Please file bugs on any testcases where there's a slowdown.  There wasn't any last time we did the numbers, so it's possible that there's a problem either in the dom binding code or in JS engine changes from 9 to 10.  It would be helpful to know which (e.g. by comparing numbers for nightlies from right before/after this landing to the numbers for both 9 and 10 and posting that information in the bugs you file).
Comment 42 Mihaela Velimiroviciu (:mihaelav) 2012-01-12 08:56:22 PST
Logged bug #717632 for test1 on Linux and bug #717637 for test2 on all platforms.
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2012-01-12 09:54:15 PST
I'd really appreciate someone doing the testing I mention in the last part of comment 41.

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