934544 - Speed up the "copy the string" case for short strings when converting strings to jsval

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We changed DOMString to use an nsAutoString in bug 920209, since that traded off one allocation (when creating the JSString) for multiple allocations while appending.

But we can avoid the allocation when creating the JSString too, for short strings: the JS engine will use a JSShortString for those as needed, and store the data inline in its GC bits.  That will also mean no need for an interesting finalizer for those strings.

All we need to do is stop manually creating the JSString and ask the JS engine to do it for us.  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: For the case when we have to copy an XPCOM string into a JSString, just ask the JS engine to do that. It'll do a better job of avoiding malloc than we can, since it can sometimes store string data inline in the string.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: For the case when we have to copy an XPCOM string into a JSString, just ask the JS engine to do that. It'll do a better job of avoiding malloc than we can, since it can sometimes store string data inline in the string.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 826852 [details] [diff] [review]
For the case when we have to copy an XPCOM string into a JSString, just ask the JS engine to do that.  It'll do a better job of avoiding malloc than we can, since it can sometimes store string data inline in the string.

\o/

Comment 4

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9957e9fe5063

Comment 5

[Tom S [:evilpie]]

Sorry, just seeing this now because of XPConnect. You seem to ignore a failure and just make that a NullValue? Actually it looks like this method is not prepared for error handling.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The API for this method blows chunks, agreed.  If you actually look at the callers, they look like this:

197                     jsval str = XPCStringConvert::ReadableToJSVal(cx, *p, &buf);
198                     if (JSVAL_IS_NULL(str))
199                         return false;

812     jsval jsstr = XPCStringConvert::ReadableToJSVal(cx, str, &sharedBuffer);
813     if (JSVAL_IS_NULL(jsstr))
814         return false;

836     jsval jsstr = XPCStringConvert::ReadableToJSVal(cx, str, &sharedBuffer);
837     if (JSVAL_IS_NULL(jsstr))
838         return false;

340     jsval jsstr = XPCStringConvert::ReadableToJSVal(ccx, aName, &buf);
341     if (JSVAL_IS_NULL(jsstr))
342         return NS_ERROR_OUT_OF_MEMORY;

Arguably we should change this method to just return a JSString*, with null signaling OOM.  I didn't want to mess with that in this bug, though, but happy to do it in a followup.

Comment 7

[Tom S [:evilpie]]

Oh sorry, didn't think we would do something like this. ^^ I can write a patch as well.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Welcome to XPConnect, and if you write it, I'll review!

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9957e9fe5063