Open Bug 1330671 Opened 3 years ago Updated 8 months ago

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

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

Tracking Status
firefox53 --- affected

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

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.
Flags: needinfo?(jdemooij)
Depends on: 966911
(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?
Flags: needinfo?(bzbarsky)
That should work, yes.  And inline js::GetStringLength is a thing already.
Flags: needinfo?(bzbarsky)
Depends on: 1334744
Flags: needinfo?(jdemooij)
Priority: -- → P2
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.