1330671 - Consider adding non-copying "fast" paths to binding JS-to-XPCOM string conversions for external strings

Status: RESOLVED DUPLICATE of bug 1591481

(Core :: DOM: Bindings (WebIDL), defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We have such fast paths in XPCConvert::JSData2Native as of bug 966911.  They look like this:

       } else if (XPCStringConvert::IsDOMString(str)) {
           nsStringBuffer::FromData((void *)chars)->ToString(length, *ws);
       } else if (XPCStringConvert::IsLiteral(str)) {
           ws->AssignLiteral(chars, length);

Two problems:

1)  We'd need to make them generic over whatever types AssignJSString can be
    passed.  This is not too bad.
2)  Those Is* tests are slow.  They require two function calls into the JS
    engine in the success case, one function call in the non-external-string
    case.  XPCConvert doesn't care much, but bindings do.  For short strings,
    those calls are likely worse than just copying the string, in terms of CPU
    usage.

We could condition the non-copying checks on the string length.  Alternately, maybe we could get inline versions of JS_IsExternalString/JS_GetExternalStringFinalizer/JS_GetTwoByteExternalStringChars in JSAPI?  At first glance this should be doable; we already have a shadow::String.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> We could condition the non-copying checks on the string length. 
> Alternately, maybe we could get inline versions of
> JS_IsExternalString/JS_GetExternalStringFinalizer/
> JS_GetTwoByteExternalStringChars in JSAPI?  At first glance this should be
> doable; we already have a shadow::String.

Hm. We could add an (inline) API like this:

  bool IsExternalString(JSString* str, JSExternalStringFinalizer** fin, const char16_t** chars);

Then the bindings code can do:

  JSExternalStringFinalizer* fin;
  const char16_t* chars;

  if (js::IsExternalString(str, &fin, &chars)) {
     // Fast path based on |fin| and |chars|.
  }

Would that work?

Comment 2

[Boris Zbarsky [:bzbarsky]]

That should work, yes.  And inline js::GetStringLength is a thing already.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This got fixed in bug 1591481.