Bug 109694 – Add support for convenient conversion from JSStringRef to QString

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Hausmann]

[Qt] Add support for convenient conversion from JSStringRef to QString

[Simon Hausmann]

Created attachment 188081 [details]
Patch

[WebKit Review Bot]

Attachment 188081 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSStringRefQt.cpp', u'Source/JavaScriptCore/API/JSStringRefQt.h', u'Source/JavaScriptCore/API/OpaqueJSString.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Target.pri']" exit_code: 1
Source/JavaScriptCore/API/JSStringRefQt.h:41:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 188081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188081&action=review

Honestly, that looks like an early optimization.


> Source/JavaScriptCore/API/OpaqueJSString.h:87
> +#if PLATFORM(QT)
> +        // If we're going to make a deep copy for use in the API layer,
> +        // then we might as well store it in a format particularly
> +        // suitable for "export" later.
> +        m_string = QString(string);
> +#else
>          // Make a copy of the source string.
>          if (string.is8Bit())
>              m_string = String(string.characters8(), string.length());
>          else
>              m_string = String(string.characters16(), string.length());
> +#endif
>      }

This should probably be better replaced by
    m_string = string.isolatedCopy()

You can then make a special case for BufferAdoptedQString.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 188081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188081&action=review
> 
> Honestly, that looks like an early optimization.
> 
> 
> > Source/JavaScriptCore/API/OpaqueJSString.h:87
> > +#if PLATFORM(QT)
> > +        // If we're going to make a deep copy for use in the API layer,
> > +        // then we might as well store it in a format particularly
> > +        // suitable for "export" later.
> > +        m_string = QString(string);
> > +#else
> >          // Make a copy of the source string.
> >          if (string.is8Bit())
> >              m_string = String(string.characters8(), string.length());
> >          else
> >              m_string = String(string.characters16(), string.length());
> > +#endif
> >      }
> 
> This should probably be better replaced by
>     m_string = string.isolatedCopy()
> 
Well, the existing code would indeed be better using isolatedCopy so it wouldn't copy literals, but wouldn't String objects adopted from QString still be better for all other cases?

[Simon Hausmann]

(In reply to comment #3)
> (From update of attachment 188081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188081&action=review
> 
> Honestly, that looks like an early optimization.

I originally did this patch as the result of optimizing the QObject bridge, which became slower after the port to the JSC C API and our customers complained when switching to Qt 5. So I sat down and looked a the profile of various use-cases and found that we really do spend a lot of time allocating memory for string copies.

But let's see, maybe I'm missing something:

I can see three paths a JSStringRef can take after its creation:

    (1) It is passed back into the C API layer (for example for JSObjectGetProperty) and in the overwhelming number of cases it is used as identifier.
    (2) It is operated on directly on using for example JSStringIsEqual(ToUTF8CString).
    (3) It is used in the calling layer, where with a high probability that it is converted to a native string type (CFStringRef, QString, etc.)

The third case involves an extra copy of data that I would like to eliminate for the Qt port, because I believe that it is particularly common. I don't believe the second use-case is affected by this, especially given that JSStringIsEqualToUTF8CString creates a temporary of the const char *. The first case - use of the JSStringRef as identifier - I suppose might affect performance negatively if the original string was an 8-bit identifier and the new one will always be 16-bit (due to QString) ?

I don't know the identifier code well enough to say for sure if that's the case and how identical 8 and 16 bit identifiers are handled. Do you know how that works?
 
> > Source/JavaScriptCore/API/OpaqueJSString.h:87
> > +#if PLATFORM(QT)
> > +        // If we're going to make a deep copy for use in the API layer,
> > +        // then we might as well store it in a format particularly
> > +        // suitable for "export" later.
> > +        m_string = QString(string);
> > +#else
> >          // Make a copy of the source string.
> >          if (string.is8Bit())
> >              m_string = String(string.characters8(), string.length());
> >          else
> >              m_string = String(string.characters16(), string.length());
> > +#endif
> >      }
> 
> This should probably be better replaced by
>     m_string = string.isolatedCopy()
> 
> You can then make a special case for BufferAdoptedQString.

You're right, isolatedCopy seems like a better fitting call to make.

As you can see however I'm less interested in the case where the underlying WTFString was previously created from a QString than in the case where we're only going to use the JSStringRef for a QString later. So I'm going to propose an isolatedCopyForAPI() to make it clearer what the copy is "optimized" for.

[Simon Hausmann]

Created attachment 188306 [details]
Patch

[Simon Hausmann]

Created attachment 188307 [details]
Patch

[WebKit Review Bot]

Attachment 188307 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSStringRefQt.cpp', u'Source/JavaScriptCore/API/JSStringRefQt.h', u'Source/JavaScriptCore/API/OpaqueJSString.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Target.pri', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/WTFString.cpp', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1
Source/JavaScriptCore/API/JSStringRefQt.h:41:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Sorry, I won't have time to review this one tonight. :( 
I thought you were doing that for DumpRenderTree, which is why I thought that was overkill.

General comments:
I think isolatedCopyForAPI does not belong is WTF unless you also need it for WebKit2. It could probably just be an inline function in a Qt header. I would prefer it not being in WTFString.h, but if it has to be, I would prefer as a helper function instead of a member (because WTF::String is already too long and people do not always bother searching for the right tools for the job in long headers).

I think your comment should mention that QString ref-count is thread safe, which is why it is safe to do a String(QStringData)->QString conversion without actually copying the data. Otherwise the code looks wrong if you only know about BufferAdoptedQString.

Don't worry about the identifiers being 16 bits strings. The part you are gonna pay is the lack of hash in the StringImpl.

It would be good to have numbers in the ChangeLog to back the perf gain.

[Simon Hausmann]

(In reply to comment #9)
> Sorry, I won't have time to review this one tonight. :( 
> I thought you were doing that for DumpRenderTree, which is why I thought that was overkill.

In DRT the patch adds convenience :)
 
> General comments:
> I think isolatedCopyForAPI does not belong is WTF unless you also need it for WebKit2. It could probably just be an inline function in a Qt header. I would prefer it not being in WTFString.h, but if it has to be, I would prefer as a helper function instead of a member (because WTF::String is already too long and people do not always bother searching for the right tools for the job in long headers).

Well, the optimization doesn't _require_ any change in WTFString.h at all. The question is: Where to place the #if PLATFORM(QT) ?

In the original patch it was placed in OpaqueJSString.h. I can leave it there if you prefer, a bit like this (pseudo):

OpaqueJSString(const String& string)
{
#if PLATFORM(QT)
    // Comment here about why and atomic refcounting, etc.
    m_string = QString(string);
#else
    m_string = string.isolatedCopy();
#endif
}
 
> I think your comment should mention that QString ref-count is thread safe, which is why it is safe to do a String(QStringData)->QString conversion without actually copying the data. Otherwise the code looks wrong if you only know about BufferAdoptedQString.

Good point, I'll stress that in the comment(s).
 
> Don't worry about the identifiers being 16 bits strings. The part you are gonna pay is the lack of hash in the StringImpl.

Ohh, ok.
 
> It would be good to have numbers in the ChangeLog to back the perf gain.

Will add some.

[Benjamin Poulain]

> In the original patch it was placed in OpaqueJSString.h. I can leave it there if you prefer, a bit like this (pseudo):
> 
> OpaqueJSString(const String& string)
> {
> #if PLATFORM(QT)
>     // Comment here about why and atomic refcounting, etc.
>     m_string = QString(string);
> #else
>     m_string = string.isolatedCopy();
> #endif
> }

Back to where we started :)

I think that is right for now.
That is a nice tool you invented and it could be useful elsewhere in the future. In that case we'll move the function to a good place.

[Simon Hausmann]

Comment on attachment 188307 [details]
Patch

Taking this out of the review queue. I need to do some more benchmarking (found some cases where it didn't work quite so well), so I'm going to split this up into two. One patch that adds the convenience (no extra cost since we're already paying for it anyway) and a separate patch/bug for the optimization itself.

[Simon Hausmann]

Created attachment 190728 [details]
Patch

[WebKit Review Bot]

Attachment 190728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSStringRefQt.cpp', u'Source/JavaScriptCore/API/JSStringRefQt.h', u'Source/JavaScriptCore/API/OpaqueJSString.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Target.pri']" exit_code: 1
Source/JavaScriptCore/API/JSStringRefQt.h:41:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Created attachment 192725 [details]
Patch

[WebKit Review Bot]

Attachment 192725 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSStringRefQt.cpp', u'Source/JavaScriptCore/API/JSStringRefQt.h', u'Source/JavaScriptCore/API/OpaqueJSString.h', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Target.pri']" exit_code: 1
Source/JavaScriptCore/API/JSStringRefQt.h:41:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

Comment on attachment 192725 [details]
Patch

r=me

[Simon Hausmann]

Committed r145700: <http://trac.webkit.org/changeset/145700>