311582 - XPCVariant for a JSString shouldn't copy the string data

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

XPCVariant::InitializeData calls 

nsVariant::SetFromWStringWithSize(&mData, 
                     (PRUint32)JS_GetStringLength(JSVAL_TO_STRING(mJSVal)),
                     (PRUnichar*)JS_GetStringChars(JSVAL_TO_STRING(mJSVal)));

Unfortunately, that makes a copy of the data.  Since XPCVariant marks the
JSString involved as a GC root while it lives, I see no reason for it to also
make a copy of the string's data.  It should just stash away the PRUnichar* and
the length in mData without copying.

Note that this means it'll have to be careful not to free the data when mData
goes away, so we may need a new mType ("dependent wstring" or something, which
claims to be WSTRING to the outside world).

As far as perf impact, jprof sez that of the 7839 hits under
XPCVariant::newVariant, 1755 are under nsMemory::Clone cloning the string.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to avoid rooting and just copy the string (no good)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to avoid copying and just root

Comment 3

[Boris Zbarsky [:bzbarsky]]

I profiled both patches, and the "avoid rooting" patch actually seems faster.  The ctor is about the same for both, but the dtor is a lot faster if it's basically just a free() call than if it's a root removal...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238605 [details] [diff] [review]
Patch to avoid rooting and just copy the string (no good)

So I think we want to take this one

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 238606 [details] [diff] [review]
Patch to avoid copying and just root

r+sr=jst

Comment 6

[Boris Zbarsky [:bzbarsky]]

jst, is there a reason you prefer the patch that tested out slower?  See comment 3.  That's why I asked for reviews on the other one...

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 238605 [details] [diff] [review]
Patch to avoid rooting and just copy the string (no good)

r+sr=jst, I got the two patches confused here and meant to review this one in the first place.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 9

[Stefan Sitter [:ssitter]]

This check-in might be the cause for the crashes in Bug 353505.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Backed that patch out; see bug 353505 comment 7 for why it was a problem.

I guess we should go with the "root and don't copy" approach...

Brendan, how much would it hurt to use JS_LockGCThing here instead of JS_AddNamedRoot?  Locking a string is a _lot_ faster than adding a gc root (no hashttable lookup).  Unlocking is about the same speed as removing a root, since both do a hashtable lookup...  But I've been told that JS_LockGCThing hurts perf, though no one's explained that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238606 [details] [diff] [review]
Patch to avoid copying and just root

I guess we should do this, pending the JS_LockGCThing business.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Backed that patch out; see bug 353505 comment 7 for why it was a problem.
> 
> I guess we should go with the "root and don't copy" approach...
> 
> Brendan, how much would it hurt to use JS_LockGCThing here instead of
> JS_AddNamedRoot?  Locking a string is a _lot_ faster than adding a gc root (no
> hashttable lookup).  Unlocking is about the same speed as removing a root,
> since both do a hashtable lookup...

We could avoid that lookup.  File a bug?

> But I've been told that JS_LockGCThing
> hurts perf, though no one's explained that.

Says who?  Hurts how?

Not AFAIK.

/be 

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We could avoid that lookup.  File a bug?

Will do.

> Says who?  Hurts how?

I seem to recall timeless and you both saying that it impacted GC perf or something to that effect.  It was never very clear to me; I'd be happy if it turns out to be wrong.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Filed bug 353602.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #13)
> > We could avoid that lookup.  File a bug?
> 
> Will do.

Thanks.
 
> > Says who?  Hurts how?
> 
> I seem to recall timeless and you both saying that it impacted GC perf or
> something to that effect.  It was never very clear to me; I'd be happy if it
> turns out to be wrong.  ;)

I don't think I've ever said that.  Locking (pinning is a better word) a GC-thing is similar to rooting a pointer or jsval, but without the indirection, and with lower cost for shallow gc-thing types as you noticed.

New patch coming?  Latest patch is obsolete, yet has review requests.

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

> New patch coming? 

Er yes.  Sorry about that.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: No copying, lock GC things

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 239469 [details] [diff] [review]
No copying, lock GC things


>+    // If mJSVal is JSVAL_STRING, we don't need to clean anything up;
>+    // simply unrooting the string is good.

s/unrooting/unlocking/ -- or "unpinning".

>+    if(JSVAL_IS_GCTHING(mJSVal))
>     {
>         JSRuntime* rt;
>         nsIJSRuntimeService* rtsrvc = nsXPConnect::GetJSRuntimeService();
> 
>         if(rtsrvc && NS_SUCCEEDED(rtsrvc->GetRuntime(&rt)))
>-            JS_RemoveRootRT(rt, &mJSVal);
>+            JS_UnlockGCThingRT(rt, JSVAL_TO_GCTHING(mJSVal));

Getting the service is not the fastest way -- is there no ccx param?  See below.

>+        // use JS_LockGCThingRT, because we get better performance in a lot of
>+        // cases than with adding a named GC root.
>+        // XXXbz I wonder whether we should use the JSRuntime from the
>+        // JSContext intstead.... and whether we should save it in a member to
>+        // make ~XPCVariant faster.
>         if(NS_FAILED(ccx.GetRuntime()->GetJSRuntimeService()->GetRuntime(&rt))||

Getting the JSContext * and using JS_LockGCThing should be faster.  The JS engine will just use cx->runtime and call the common subroutine.

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

> is there no ccx param?

It's a destructor.  There are no params.

I could save the JSRuntime in a member, as I said.  Is that safe?

Comment 20

[Brendan Eich [:brendan]]

Sure, the runtime is around until shutdown.

/be

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 239520 [details] [diff] [review]
Updated to comments

r=jst

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked that fix in on trunk.