1279746 - Optimize xpc_TryUnmarkWrappedGrayObject

Status: RESOLVED FIXED

Description

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

Created attachment 8762328 [details] [diff] [review]
wip

To reduce some useless, yet slow AddRef/Release, we could just unmark gray inside QI.
Tiny bit ugly approach, but not that different to getting nsCycleCollectionISupports for example.

nsIXPConnectWrappedJSUnmarkGray extends nsIXPConnectWrappedJS so that we don't end up increasing the size of the object.

Addref/release does show up in some profiles when one has hundreds of tabs and we're executing forgetSkippable.

Comment 1

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

Comment on attachment 8762328 [details] [diff] [review]
wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf67b9f959502ba463aa483b555a0f3ebcd9d4e

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8762328 [details] [diff] [review]
wip

Review of attachment 8762328 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +129,5 @@
>  
>  };
>  
> +// Special interface to unmark the internal JSObject.
> +// QIing to nsIXPConnectWrappedJSUnmarkGray does *not* addref, it only unmarks!

Maybe say that it returns null and fails when the QI happens, too?

@@ +130,5 @@
>  };
>  
> +// Special interface to unmark the internal JSObject.
> +// QIing to nsIXPConnectWrappedJSUnmarkGray does *not* addref, it only unmarks!
> +[uuid(c02a0ce6-275f-4ea1-9c23-08494898b070)]

This should have some kind of indicator saying that it is only implementable by C++. "native" maybe?

::: js/xpconnect/src/nsXPConnect.cpp
@@ +340,5 @@
>  
>  void
>  xpc_TryUnmarkWrappedGrayObject(nsISupports* aWrappedJS)
>  {
> +    nsCOMPtr<nsIXPConnectWrappedJSUnmarkGray> wjsug =

I think you could save a branch by making this a raw pointer instead of an nsCOMPtr. I'll leave it up to you whether that is worthwhile or not. ;) Do you need a DebugOnly on this to prevent warning in opt builds?

::: js/xpconnect/src/xpcprivate.h
@@ +2337,5 @@
>  public:
>      NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>      NS_DECL_NSIXPCONNECTJSOBJECTHOLDER
>      NS_DECL_NSIXPCONNECTWRAPPEDJS
> +    NS_DECL_NSIXPCONNECTWRAPPEDJSUNMARKGRAY

Can you get rid of NS_DECL_NSIXPCONNECTWRAPPEDJS?

Comment 3

[Nathan Froyd [:froydnj]]

You can put |builtinclass| on the IDL class to make sure JS doesn't implement it.

Comment 4

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> ::: js/xpconnect/src/xpcprivate.h
> @@ +2337,5 @@
> >  public:
> >      NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> >      NS_DECL_NSIXPCONNECTJSOBJECTHOLDER
> >      NS_DECL_NSIXPCONNECTWRAPPEDJS
> > +    NS_DECL_NSIXPCONNECTWRAPPEDJSUNMARKGRAY
> 
> Can you get rid of NS_DECL_NSIXPCONNECTWRAPPEDJS?
No. NS_DECL_ are generated per interface, afaik. In this case NS_DECL_NSIXPCONNECTWRAPPEDJSUNMARKGRAY
should be empty, but I put it there for consistency.

Comment 5

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

> >  xpc_TryUnmarkWrappedGrayObject(nsISupports* aWrappedJS)
> >  {
> > +    nsCOMPtr<nsIXPConnectWrappedJSUnmarkGray> wjsug =
> 
> I think you could save a branch by making this a raw pointer instead of an
> nsCOMPtr.
Hmm, well, that would mean manual QueryInterface call I think. Or can somehow trick do_QueryInterface to play with raw pointers

> Do
> you need a DebugOnly on this to prevent warning in opt builds?
What warning?

Comment 6

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

Created attachment 8762667 [details] [diff] [review]
+comments

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ca0a41a661
Optimize xpc_TryUnmarkWrappedGrayObject, r=mccr8

Comment 8

[Andrew McCreight [:mccr8]]

> What warning?
Unused variable.

Comment 9

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

oh, I see. Hmm. Do we actually warn about it? We have kungfuDeathGrips everywhere and this is similar.

Comment 10

[Nathan Froyd [:froydnj]]

The variable is "used" in the sense that it has non-trivial constructors and destructors.  So that's why kungFuDeathGrip doesn't warn and why this doesn't warn.  If we went the raw pointer direction, we'd have to do a DebugOnly<>.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3ca0a41a661