Make inline style use wrapper cache and slimwrappers

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
DOM
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Assigned: peterv)

Tracking

Trunk
mozilla1.9.2a1
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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

Comment 1

9 years ago
Created attachment 387528 [details] [diff] [review]
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.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #387528 - Flags: superreview?(bzbarsky)
Attachment #387528 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

9 years ago
Created attachment 387529 [details] [diff] [review]
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.
(Assignee)

Comment 3

9 years ago
Created attachment 387530 [details] [diff] [review]
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 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.
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?
(Assignee)

Comment 6

9 years ago
(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.
> 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 on attachment 387528 [details] [diff] [review]
Part 1 v1

Per discussion.
Attachment #387528 - Flags: superreview?(bzbarsky)
Attachment #387528 - Flags: superreview+
Attachment #387528 - Flags: review?(bzbarsky)
Attachment #387528 - Flags: review+
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.
(Assignee)

Comment 10

9 years ago
Created attachment 388668 [details] [diff] [review]
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?).
Attachment #387528 - Attachment is obsolete: true
Attachment #388668 - Flags: superreview+
Attachment #388668 - Flags: review+
(Assignee)

Comment 11

9 years ago
Created attachment 388669 [details] [diff] [review]
Part 3 v1

Updated patch, this one should link.
Attachment #387530 - Attachment is obsolete: true
Yeah, that seems to run fine; drops times from the 815ms range to 770ms range on that testcase.
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.
It might be worth spinning off the other parts into separate bugs, then, just so we can track the state of things.
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?
Blocks: 505217
(Assignee)

Updated

9 years ago
Blocks: 508780
(Assignee)

Comment 17

9 years ago
Created attachment 392932 [details] [diff] [review]
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.
Attachment #387529 - Attachment is obsolete: true
Attachment #388669 - Attachment is obsolete: true
Attachment #392932 - Flags: superreview?(bzbarsky)
Attachment #392932 - Flags: review?(bzbarsky)
Attachment #392932 - Flags: superreview?(bzbarsky)
Attachment #392932 - Flags: superreview+
Attachment #392932 - Flags: review?(bzbarsky)
Attachment #392932 - Flags: review+
(Assignee)

Comment 18

9 years ago
Attachment 392932 [details] [diff] landed.

http://hg.mozilla.org/mozilla-central/rev/a8c89c6a48e6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.