519608 - Is it worth optimizing for the inline style case in nsCSSStyleDeclSH::PreCreate?

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, nsCSSStyleDeclSH::PreCreate does this:

  jsval v;
  nsresult rv = WrapNative(cx, globalObj, native_parent, PR_FALSE, &v);
  NS_ENSURE_SUCCESS(rv, rv);

But in almost all cases, |native_parent| is an Element (HTML/SVG/XUL) and already has a wrapper (and in fact, we're just getting a property off that wrapper).  This only matters the first time we wrap a given CSSDeclaration, but that time this WrapNative call is almost half of our slimwrapper construction time.  Would it make sense to optimize for the case when native_parent implements nsWrapperCache and can just hand out a jsobject, falling back on WrapNative if for some reason (e.g. someone manually tried to wrap a CSSDeclaration from C++) that's not the case?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Maybe like so

I think this should be safe, since wrapper-cached stuff doesn't care about the scope you passed in (certainly not once the wrapper is created), right?

It certainly speeds up my testcase (which does lots of creating nodes and setting style on them) up by 3% or so.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I guess we could nix the QI to nsWrapperCache too, by changing the relevant GetParentObject signatures to return nsINode.  That won't work if we ever start wrapper-caching CSSStyleRule declarations, though.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 403693 [details] [diff] [review]
Maybe like so

We could even avoid the QI for some of these parents (for example nsINode inherits from nsWrapperCache, no need to QI if we have an nsINode). We'd have to change some of the functions that get the parent object to return a more specific interface.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Right; that's what I said in comment 2.  Do you want me to do that?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4e46887f17bc

Do we want a separate bug on comment 2?

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> Right; that's what I said in comment 2.  Do you want me to do that?

Sorry, I just read the request bugmail before reviewing.

(In reply to comment #5)
> Do we want a separate bug on comment 2?

Yeah, the QI will probably not stick out in a profile, but it might at some point.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Filed bug 519905 on getting rid of those QIs.