Closed
Bug 311582
Opened 19 years ago
Closed 18 years ago
XPCVariant for a JSString shouldn't copy the string data
Categories
(Core :: XPConnect, defect, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jst
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.9a2?
Updated•19 years ago
|
QA Contact: pschwartau → xpconnect
Updated•18 years ago
|
Flags: blocking1.9a2? → blocking1.9-
Whiteboard: [wanted-1.9]
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
Assignee | ||
Comment 3•18 years ago
|
||
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...
Assignee: dbradley → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 4•18 years ago
|
||
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
Attachment #238605 -
Flags: superreview?(jst)
Attachment #238605 -
Flags: review?(jst)
Comment 5•18 years ago
|
||
Comment on attachment 238606 [details] [diff] [review]
Patch to avoid copying and just root
r+sr=jst
Attachment #238606 -
Flags: superreview+
Attachment #238606 -
Flags: review+
Updated•18 years ago
|
Attachment #238605 -
Flags: superreview?(jst)
Attachment #238605 -
Flags: review?(jst)
Assignee | ||
Comment 6•18 years ago
|
||
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•18 years ago
|
||
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.
Attachment #238605 -
Flags: superreview+
Attachment #238605 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Attachment #238606 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 9•18 years ago
|
||
This check-in might be the cause for the crashes in Bug 353505.
Assignee | ||
Comment 10•18 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•18 years ago
|
Attachment #238605 -
Attachment description: Patch to avoid rooting and just copy the string → Patch to avoid rooting and just copy the string (no good)
Attachment #238605 -
Attachment is obsolete: true
Assignee | ||
Comment 11•18 years ago
|
||
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.
Attachment #238606 -
Flags: superreview?(brendan)
Attachment #238606 -
Flags: superreview+
Attachment #238606 -
Flags: review?(jst)
Attachment #238606 -
Flags: review+
Comment 12•18 years ago
|
||
(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
Assignee | ||
Comment 13•18 years ago
|
||
> 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. ;)
Assignee | ||
Comment 14•18 years ago
|
||
Filed bug 353602.
Comment 15•18 years ago
|
||
(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
Assignee | ||
Comment 16•18 years ago
|
||
> New patch coming?
Er yes. Sorry about that.
Assignee | ||
Comment 17•18 years ago
|
||
Attachment #239469 -
Flags: superreview?(brendan)
Attachment #239469 -
Flags: review?(jst)
Assignee | ||
Updated•18 years ago
|
Attachment #238606 -
Flags: superreview?(brendan)
Attachment #238606 -
Flags: review?(jst)
Comment 18•18 years ago
|
||
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
Assignee | ||
Comment 19•18 years ago
|
||
> 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•18 years ago
|
||
Sure, the runtime is around until shutdown.
/be
Assignee | ||
Comment 21•18 years ago
|
||
Attachment #239469 -
Attachment is obsolete: true
Attachment #239520 -
Flags: superreview?(brendan)
Attachment #239520 -
Flags: review?(jst)
Attachment #239469 -
Flags: superreview?(brendan)
Attachment #239469 -
Flags: review?(jst)
Comment 22•18 years ago
|
||
Comment on attachment 239520 [details] [diff] [review]
Updated to comments
r=jst
Attachment #239520 -
Flags: review?(jst) → review+
Updated•18 years ago
|
Attachment #239520 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 23•18 years ago
|
||
Checked that fix in on trunk.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
You need to log in
before you can comment on or make changes to this bug.
Description
•