Optimize xpc_TryUnmarkWrappedGrayObject

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments)

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 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?
Attachment #8762328 - Flags: review?(continuation) → review+
Assignee: nobody → bugs
You can put |builtinclass| on the IDL class to make sure JS doesn't implement it.
(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.
> >  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 7

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ca0a41a661
Optimize xpc_TryUnmarkWrappedGrayObject, r=mccr8
> What warning?
Unused variable.
oh, I see. Hmm. Do we actually warn about it? We have kungfuDeathGrips everywhere and this is similar.
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3ca0a41a661
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.