481677 - Avoid hash lookups in XPCWrappedNative::GetNewOrUsed

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

When we have a nsWrapperCache and we get into XPCWrappedNative::GetNewOrUsed we're sure that we don't have a wrapper, so no need to look it up in the hash.
We can also avoid an addref/release pair on the identity object if we transfer the pointer from the local identity nsCOMPtr to the wrapper instead of addrefing in the wrapper and releasing from the nsCOMPtr.

This came up in bug 481131, comment 11. Bz, any chance you could try if this patch helps with that testcase?

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, on the same toc.js the breakdown for GetNewOrUsed now is:

7% self
20% XPCWrappedNative::FindTearOff
9% XPCWrappedNative::Init
9% XPCWrappedNativeProto::GetNewOrUsed
8% QI
8% nsDOMClassInfo::PostCreate
7% operator new
4% JS_DHashTableOperate
4% XPCWrappedNative::GatherScriptableCreateInfo
4% nsElementSH::PostCreate
3% nsContentListSH::PreCreate
2% PR_EnterMonitor
1% PR_ExitMonitor
1% PR_AtomicIncrement
1% nsNodeSH::PreCreate
0.6% ~nsCOMPtr

So it looks like this does help with the extra release (cycle collector stuff) and reduces the JS_DHashTableOperate time.

There might have been a 1-2% win on the wall-clock time on toc.js and dfn.js.  Hard to say because the test is so noisy....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, less monitor time used too; presumably due to not locking around the hashtable get.

Would it make sense to pass in an already_AddRefed instead of an nsCOMPtr, and then pass forget() on the relevant comptr and just do an initializer-style assign in the constructor?

Comment 3

[Boris Zbarsky [:bzbarsky]]

That would make it very clear that |identity| is null, fwiw.

Comment 4

[Peter Van der Beken [:peterv]]

It would, yes. I gave up on it after I found out that the specialized nsCOMPtr<nsISupports> doesn't have the forget() that returns already_AddRefed. I guess I can add it, wanted to minimize the changes so we could take this on branch, but that's fairly risk-free.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 365872 [details] [diff] [review]
v1.1

Want to add "unified = 8" to your [diff] section in .hgrc so that you mq patches have nice context?

>+++ b/js/src/xpconnect/src/xpcprivate.h
>     GetNewOrUsed(XPCCallContext& ccx,
>                  nsISupports* Object,
>                  XPCWrappedNativeScope* Scope,
>                  XPCNativeInterface* Interface,
>+                 nsWrapperCache* cache,

Document that cache can only be non-null if cache->GetWrapper() is null?

>+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
> XPCWrappedNative::GetNewOrUsed(XPCCallContext& ccx,

And assert that at entry to this function, please.

I'd like to see a diff -w of this file, with the 8-line context; reviewing the diff as it stands is pretty hard.

Other than that, looks good.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.2

(In reply to comment #6)
> Document that cache can only be non-null if cache->GetWrapper() is null?

I've added a comment that the nsWrapperCache should be passed in (not an error if it isn't) and that if a cache is passed in then cache->GetWrapper() must be null.

> And assert that at entry to this function, please.

Moved the assert to the top of the function.

> I'd like to see a diff -w of this file, with the 8-line context; reviewing the
> diff as it stands is pretty hard.

I'd tried diff -w, it doesn't make it any clearer but here it is.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That looks like an hg diff -w.  How about something like this:

[extensions]
extdiff =

[extdiff]
cmd.wdiff = diff
opts.wdiff = -p -U 8 -w -r -N

Then wdiff the file?

Comment 9

[Boris Zbarsky [:bzbarsky]]

(oh and hg's built in -w option to diff is known-broken).

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1.2 (diff -w)

Ah, better.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 365986 [details] [diff] [review]
v1.2

- In XPCWrappedNative::GetNewOrUsed():

-        wrapper = new XPCWrappedNative(identity, proto);
+        wrapper = new XPCWrappedNative(identity.forget(), proto);
         if(!wrapper)
             return NS_ERROR_FAILURE;
     }
     else
     {
         AutoMarkingNativeSetPtr set(ccx);
         set = XPCNativeSet::GetNewOrUsed(ccx, nsnull, Interface, 0);
 
         if(!set)
             return NS_ERROR_FAILURE;
 
-        wrapper = new XPCWrappedNative(identity, Scope, set);
+        wrapper = new XPCWrappedNative(identity.forget(), Scope, set);

Those two changes will cause a leak if we run out of memory right? Also, I think the fact that identity is nulled out from this point on is worthy of a comment so we don't down the road end up trying to use identity later on in this function.

r=jst with that looked into.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Had to back out due to orange, needs a small change: we need to addref the wrapper if we get it from the cache after the call to PreCreate.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 368280 [details] [diff] [review]
v1.3

Would like to take this on branch, it's a small performance improvement in XPConnect. There's no testsuite for the performance improvement, our existing testsuites already cover the correctness of this change.

Comment 14

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/7262dbfe96fb

Comment 15

[Robert Sayre]

(In reply to comment #13)
> (From update of attachment 368280 [details] [diff] [review])

> There's no testsuite for the performance improvement

Can you attach a test case? I am working on something to cover stuff like this.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 368280 [details] [diff] [review]
v1.3

a191=drivers. Would like to see more comprehensive testing, but taking your assurances (and sayrer's comment).

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cfb24602548b

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

This apparently caused a Tp reduction on XP - yay, you!

Comment 19

[Justin Dolske [:Dolske]]

And also apparently caused a Twinopen regression on XP. :-( See "Talos Regression: Txul increase 3.56% on XP 1.9.1" thread in m.d.tree-management.

Backed out, watching to see if the regression goes away.

Backout: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d47dbd3488f9

Comment 20

[Peter Van der Beken [:peterv]]

This wasn't backed out on trunk afaict.

Comment 21

[Justin Dolske [:Dolske]]

Hmm, well, now that you mention it it also looks like Twinopen regressed on trunk as well:

http://graphs.mozilla.org/#show=2420846,2419200,2417613

The automated scripts didn't alert about it, but it looks like a regression. Oddly, it's dropped back down today, but there were some landings in the window that could potentially have improved things.

So, I think this should be unfortunately be backed out on trunk as well.

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 368280 [details] [diff] [review]
v1.3

Txul regression fixed. I'm not going to track down why, the patch is simple enough and I don't see anything obvious that can cause that regression. Clearing 1.9.1 approval.

Comment 23

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> Hmm, well, now that you mention it it also looks like Twinopen regressed on
> trunk as well:
> 
> http://graphs.mozilla.org/#show=2420846,2419200,2417613

> So, I think this should be unfortunately be backed out on trunk as well.

I'll try that, but I don't really see a clear regression on trunk around the time that this patch landed (March 19th).

Comment 24

[Justin Dolske [:Dolske]]

Doh. Sorry, my mistake. :-( I didn't notice this landed on trunk almost exactly 1 month before it landed on branch. The comments here on 3/22 and 4/21 are not 1 day apart! And the coincidental clear regression on trunk is not so coincidental after all, because the graph I linked to is for branch.

So, I'm not sure WTF I was smoking. Trunk looks fine, no backout needed.

*sigh*