500850 - Make inline style use wrapper cache and slimwrappers

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

As the summary says: make inline style use wrapper cache and slimwrappers.  Should help scripts that do foo.style.top all the time...

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Part 1 v1

This adds cached wrappers to nsDOMCSSAttrDeclaration and nsComputedDOMStyle, but not to DOMCSSDeclarationImpl (in nsCSSStyleRule.cpp). Bz: it's not really hard to add it to DOMCSSDeclarationImpl, do you think it would be worth it? This also adds thisptr offset tables to nsDOMCSSDeclaration and nsComputedDOMStyle, and makes nsGlobalWindow::GetComputedStyle not go through a factory to create a nsComputedDOMStyle.
Note that because nsIDOMCSS2Properties and nsIDOMNSCSS2Properties are implemented in a tearoff the thisptr offset tables won't make a huge difference with this patch alone.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Part 2 v1

This makes better use of the thisptr offset table by autogenerating all the quickstubs for nsIDOMCSS2Properties and nsIDOMNSCSS2Properties by forwarding to semi-generated quickstubs for nsICSSDeclaration_GetPropertyValue and nsICSSDeclaration_GetPropertyValue. Not entirely happy with the fact that those are semi-generated.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Part 3 v1

If we take part 2 we can make CSS2PropertiesTearoff a real non-cached tearoff without a performance penalty when calling from JS. It would shrink nsDOMCSSDeclaration and nsComputedDOMStyle by two words.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 387528 [details] [diff] [review]
Part 1 v1

> Bz: it's not really hard to add it to DOMCSSDeclarationImpl, do you think it
> would be worth it?

I doubt it; that's not touched very often.

>+++ b/dom/base/nsDOMClassInfo.cpp

>+nsCSSStyleDeclSH::PreCreate(nsISupports *nativeObj, JSContext *cx,
>+  if (!cache) {
>+    return nsDOMClassInfo::PreCreate(nativeObj, cx, globalObj, parentObj);

Why do we not need to make an equivalent call in, say, nsNodeListSH::PreCreate?  Or is that just a bug?

>+  nsISupports *native_parent = declaration->GetParentObject();
...
>+  nsresult rv = WrapNative(cx, globalObj, native_parent, &v);

Hmm.  So doesn't this change property lookup on CSS declarations?  Or is that not an issue for some reason?  I thought the scope parent thing just affected stuff like event handlers (probably not an issue here) and with() (might be an issue).  Or are we overloading scope parents to mean something else too now?

The rest of this looks fine.

Comment 5

[Boris Zbarsky [:bzbarsky]]

David might be interested in the part 3 there....  Note that that seems to make performance a tiny worse for C++ callers, right?  Probably not an issue in any case...

Would it help with the autogeneration bit if nsICSSDeclaration were in the form of [noscript] idl or some such?

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> Why do we not need to make an equivalent call in, say, nsNodeListSH::PreCreate?
>  Or is that just a bug?

I'm not sure yet, but I think it might be. I'll file a bug about it.

> Hmm.  So doesn't this change property lookup on CSS declarations?  Or is that
> not an issue for some reason?  I thought the scope parent thing just affected
> stuff like event handlers (probably not an issue here) and with() (might be an
> issue).  Or are we overloading scope parents to mean something else too now?

I'll check this. We're not really overloading scope parent, the problem is that in order to have just one wrapper in the whole system for these objects we need a specific parent, regardless of the scope we're wrapping in. And if we want to cache wrappers we can only have one wrapper in the whole system for these objects.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> just one wrapper in the whole system for these objects we need a specific parent

Ah, right.  But _which_ parent sort of matters, as I recall... Please do check on this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 387528 [details] [diff] [review]
Part 1 v1

Per discussion.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I just tried these patches...  Part 3 doesn't link.  Parts 1+2 together seem to speed up the test in bug 229391 by 5% or so, but after running it a few times crash in cycle collection somewhere.  Not sure whether I need to have both patches applied to hit that.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Part 1 v1

The cycle collection crash was a regression from a last-minute change I made to the patch just before attaching :-/. The NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY is slightly different from others, because it doesn't have an else at the end and so we have to be careful wrt what other macros follow it (NS_IMPL_QUERY_TAIL_INHERITING sets foundInterface to 0 as its first line). I'll file a bug to figure out if we can change NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY to avoid these problems (by adding an else?).

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Part 3 v1

Updated patch, this one should link.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, that seems to run fine; drops times from the 815ms range to 770ms range on that testcase.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I just had a nice conversation with mrbkap, who told me that I'm on crack about how with() works.  It doesn't push the object it's given on the scope chain; it puts a With object which then delegates to the given object and its proto chain.  So with() behavior is not affected by an object's parent chain.  As long as we don't have DOM event handlers on inline style, this change should cause no observable behavior differences, sounds like.

Comment 14

[Peter Van der Beken [:peterv]]

Checked in part 1: http://hg.mozilla.org/mozilla-central/rev/0ab9276e2dba

Comment 15

[Boris Zbarsky [:bzbarsky]]

It might be worth spinning off the other parts into separate bugs, then, just so we can track the state of things.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 388668 [details] [diff] [review]
Part 1 v1

>+      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "slots mStyle");
>+      cb.NoteXPCOMChild(slots->mStyle.get());

Why no addition to the unlink method?

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Unlink v1

Yeah, we should unlink. I also added traversal of the script object to nsComputedDOMStyle, I don't think we currently preserve any of these wrappers (when someone sets an expando on them), but it shouldn't cost much to try to traverse them and if we ever do preserve them we're already safe.

Comment 18

[Peter Van der Beken [:peterv]]

Attachment 392932 [details] [diff] landed.

http://hg.mozilla.org/mozilla-central/rev/a8c89c6a48e6