For great speed.

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.

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

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.

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

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.

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

These are the JSAPI changes that we need for the other patches.

Your python files need license headers, and in nsWrapperCacheInlines.h, the Initial Developer is MoFo, not MoCo.

Some minor fixes.

Merged to trunk and simplified wrapper cache a little bit.

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

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.

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?

(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

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.