This uses an additional slot, so it doesn't work right now for proxy bindings, which should only be a problem when we convert form elements, I think. We can fix it at that point (probably by freeing up one of the expando slots, as bholley suggested).

Comment on attachment 689419 [details] [diff] [review] v1.1 >+++ b/dom/bindings/BindingUtils.h >+// to the cached SOW, JS::NullValue if we need to create a SOW (and cache it I'd sort of like it if we spelled out SystemOnlyWrapper here, at least once. >+++ b/js/xpconnect/wrappers/WrapperFactory.cpp >+ // be a security wrapper. These checks implicitly handles the security s/handles/handle/ Don't we need to beef up the check in WrapNewBindingObject to not return obj if it needs a SOW? I wonder whether it's faster to do that by examining the object slot or by asking the T* we have... Not marking review yet; would like to understand the WrapNewBindingObject situation.

Comment on attachment 689419 [details] [diff] [review] v1.1 Need to fix WrapNewBindingObject...

Comment on attachment 693620 [details] [diff] [review] v1.2 >+struct WrapNewBindingForSameCompartment<T, true> Are we sure the virtual function call is faster than just examining the contents of the slot? That's not obvious to me.... Have we done any measurement on that? We could keep detecting that we should use this thing using the method, and asserting that slot and method match, but use the slot as the opt-build source of truth, if it's faster than the virtual call. >+ if (CouldBeDOMBinding(value)) { >+ obj = WrapNewBindingForSameCompartment<T>::Wrap(cx, value, obj); >+ } else if (!JS_WrapObject(cx, &obj)) { >+ return false; >+ } In the short term this is introducing a JS_WrapObject on every getter that returns a node. That seems a bit unfortunate. Maybe we can at least add a fast-path for slimwrappers here, temporarily? Of course even having to check the slot is pretty unfortunate for things like .firstChild and .nextSibling, especially because we know they can't possibly need SOWs unless they're happening on a SOW to start with. I'm not sure I have a great solution to that, though, short of eliminating SOWs altogether. :( r=me with the above dealt with.

billm tells me we can assume JSObject* is 8-byte aligned, but probably shouldn't assume it's 16-byte aligned. So we do have one more flag bit in nsWrapperCache if we need...

This uses a wrappercache flag and eagerly creates the SOW in nsINode::WrapObject, which I think is fine.

Comment on attachment 694157 [details] [diff] [review] v2 >+++ b/dom/bindings/BindingUtils.h > WrapNewBindingObject(JSContext* cx, JSObject* scope, T* value, JS::Value* vp) >+ bool couldBeDOMBinding = CouldBeDOMBinding(value); Hmm. So we need this even on the fast path because we might have a wrappercached non-new-binding thing? OK. >- if (js::GetObjectCompartment(obj) == js::GetContextCompartment(cx)) { ... >+ bool sameCompartment = js::IsObjectInContextCompartment(obj, cx); No, please leave this test the way it was. It used to be completely inlined, while js::IsObjectInContextCompartment is not inlined. On the other hand, maybe we should file a followup to inline js::IsObjectInContextCompartment.... I'm really looking forward to this code getting simpler. :( r=me with the compartment check fixed to be fast again.

Try showed one orange, I added this fix: if (wrapper && js::GetObjectCompartment(wrapper) == js::GetObjectCompartment(scope) && - ((cache->IsDOMBinding() && !cache->HasSystemOnlyWrapper()) || - IS_SLIM_WRAPPER(wrapper) || - xpc_OkToHandOutWrapper(cache))) { + (cache->IsDOMBinding() ? !cache->HasSystemOnlyWrapper() : + (IS_SLIM_WRAPPER(wrapper) || xpc_OkToHandOutWrapper(cache)))) {

This (or something else in the same range, but probably[1] this) seems to have caused a 2-3% Dromaeo regression on multiple Windows & Mac platforms[2]... Should this be backed out? [1] https://groups.google.com/d/msg/mozilla.dev.tree-management/fwuqdzPxOp0/bbi-GugM314J [2] https://groups.google.com/d/msg/mozilla.dev.tree-management/ZWHo-O98_E0/sK1Q51dOWH0J

Unfortunately, no. The code was faster before this patch due to skipping a critical security check. :( See bug 816071 comment 28 through bug 816071 comment 32. I'm going to try to do some more profiling on this early next week when I have a profiler with sane per-line blame, I guess, but I somewhat doubt anything interesting will pop up.

That said, I'll double-check in a profiler that nothing too insane is going on here.

Yeah, no obvious function call or anything like that. Hard to tell per-line blame until I get back to a machine with Shark next week, sadly. :( That said, for cases when what's being returned is not a new-binding object, comment 113 understates the actual work. It actually involves a JSClass get and whatnot as well, so could totally be hitting some extra cache misses and losing.

> Yeah, no obvious function call or anything like that. At least on 64-bit Mac.

Yeah, I've been profiling too but nothing showed up yet.