Closed Bug 648801 Opened 13 years ago Closed 13 years ago

Prototype a proxy-based NodeList implementation

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bzbarsky, Assigned: peterv)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 13 obsolete files)

32.25 KB, patch
bzbarsky
: feedback-
Details | Diff | Splinter Review
31.83 KB, application/zip
Details
14.58 KB, patch
Details | Diff | Splinter Review
256.87 KB, patch
jst
: review+
mrbkap
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
15.58 KB, text/plain
Details
1.74 KB, text/plain
Details
For great speed.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gal
Attached patch patch (obsolete) — Splinter Review
Attachment #524898 - Attachment is obsolete: true
Still no support for item(), so the browser UI is a bit broken.
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.
Attached patch And now without the crashing (obsolete) — Splinter Review
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...
Attachment #524931 - Attachment is obsolete: true
Attached patch Without crashing for real (obsolete) — Splinter Review
Attachment #524933 - Attachment is obsolete: true
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Rollup patch (obsolete) — Splinter Review
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.
Attachment #524899 - Attachment is obsolete: true
Attachment #524930 - Attachment is obsolete: true
Attachment #524934 - Attachment is obsolete: true
(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).
> 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.
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.
(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).
Attached patch Patch as of this morning (obsolete) — Splinter Review
Attachment #525034 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #525504 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #525525 - Attachment is obsolete: true
Attachment #525532 - Attachment is obsolete: true
Attachment #525725 - Flags: feedback?(bzbarsky)
Based on timing measurements and profiles so far that cache is not working.  Rebuilding debug to figure out what's going on.
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.
Depends on: 649887
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
Attachment #525725 - Flags: feedback?(bzbarsky) → feedback-
Blocks: 665894
Depends on: 678528
Attached patch JSAPI changes v1 (obsolete) — Splinter Review
These are the JSAPI changes that we need for the other patches.
Assignee: gal → peterv
Status: NEW → ASSIGNED
Attachment #553152 - Attachment is obsolete: true
Attached patch Main patch v1 (obsolete) — Splinter Review
Attachment #553163 - Attachment description: v1.1 → JSAPI changes v1.1
Your python files need license headers, and in nsWrapperCacheInlines.h, the Initial Developer is MoFo, not MoCo.
Attached patch Main patch v1.1 (obsolete) — Splinter Review
Some minor fixes.
Attachment #553225 - Attachment is obsolete: true
Attached patch Main patch v1.2Splinter Review
Merged to trunk and simplified wrapper cache a little bit.
Attachment #554653 - Attachment is obsolete: true
Attachment #554877 - Flags: review?(jst)
Attachment #554877 - Flags: review?(mrbkap)
Attachment #554877 - Flags: review?(gal)
Attachment #554877 - Flags: review?(bzbarsky)
Blocks: 686120
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.
Attachment #554877 - Flags: review?(bzbarsky) → review+
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.
Attachment #554877 - Flags: review?(mrbkap) → review+
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.
Attachment #554877 - Flags: review?(jst) → review+
Depends on: 691472
Depends on: 691475
Depends on: 691499
Depends on: 691706
Depends on: 691707
Depends on: 693258
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 693323
This broke clang builds, see bug 693323.
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?
Depends on: 693364
(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?
This breaks profiledbuild on Linux x86_64. Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=693563
Depends on: 693563
Depends on: 693811
Depends on: 693894
Depends on: 694009
Depends on: 694308
Depends on: 694594
Attachment #554877 - Flags: review?(gal)
Depends on: 695867
Depends on: 697643
Depends on: 698549
Depends on: 698551
Depends on: 699826
Target Milestone: --- → mozilla10
Depends on: 715156
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!
> 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.
Will someone continue to work to improve the results that are worse on Firefox 10 than Firefox 9?
"Probably".  Which ones are those?
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!
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).
Logged bug #717632 for test1 on Linux and bug #717637 for test2 on all platforms.
I'd really appreciate someone doing the testing I mention in the last part of comment 41.
Depends on: 717842
Depends on: 724033
Depends on: 734002
Depends on: 759621
Depends on: 794990
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.