Closed
Bug 1279746
Opened 7 years ago
Closed 7 years ago
Optimize xpc_TryUnmarkWrappedGrayObject
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
Details
Attachments
(2 files)
4.40 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
4.49 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Comment on attachment 8762328 [details] [diff] [review] wip https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf67b9f959502ba463aa483b555a0f3ebcd9d4e
Attachment #8762328 -
Flags: review?(continuation)
Comment 2•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee: nobody → bugs
![]() |
||
Comment 3•7 years ago
|
||
You can put |builtinclass| on the IDL class to make sure JS doesn't implement it.
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
> > 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?
Assignee | ||
Comment 6•7 years ago
|
||
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ca0a41a661 Optimize xpc_TryUnmarkWrappedGrayObject, r=mccr8
Comment 8•7 years ago
|
||
> What warning?
Unused variable.
Assignee | ||
Comment 9•7 years ago
|
||
oh, I see. Hmm. Do we actually warn about it? We have kungfuDeathGrips everywhere and this is similar.
![]() |
||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c3ca0a41a661
Status: NEW → RESOLVED
Closed: 7 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.
Description
•