Bug 83243 - Speed up the conversion from JSValue to String for bulk operations
Summary: Speed up the conversion from JSValue to String for bulk operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks: 83180
  Show dependency treegraph
 
Reported: 2012-04-04 19:25 PDT by Benjamin Poulain
Modified: 2012-04-05 17:12 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.94 KB, patch)
2012-04-04 19:36 PDT, Benjamin Poulain
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-04-04 19:25:57 PDT
For operations on a large number of string, a lot of time is spent converting values (usually primitive) to String.

Most of the time is spent allocating and deallocating JSString.
A significant part of the time is function overhead of calling to string.

We can do much better for primitive types.
Comment 1 Benjamin Poulain 2012-04-04 19:36:23 PDT
Created attachment 135748 [details]
Patch
Comment 2 Benjamin Poulain 2012-04-04 19:39:49 PDT
Extra notes:

-This is on top of https://bugs.webkit.org/show_bug.cgi?id=83180
Disregard the bots.

-I have only converted the two hotspot I was hitting to make things simple. In a followup, I should convert all the others "toString(exec)->value(exec)" to either toUString() or element.toUStringInline() depending on the case.

-This makes no difference for JSString. This only improves the other primitives types.
Comment 3 Geoffrey Garen 2012-04-04 22:08:45 PDT
Comment on attachment 135748 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        When making operations on primitive types, we loose some time converting
> +        values to JSString in order to extract the string.

It looks like JSValue::toString only artificially creates a JSString if the JSValue is a number. I wonder if NumericStrings should just change to cache JSStrings instead of UStrings, and how much of this issue that would resolve.

> Source/JavaScriptCore/ChangeLog:11
> +        This patch speed up some basic Array operations by avoiding the creation

Grammar: speeds.

> Source/JavaScriptCore/ChangeLog:15
> +        an inline convertion is used.

Spelling: conversion.

> Source/JavaScriptCore/runtime/JSString.h:472
> +    ALWAYS_INLINE UString inlineJSValueNotStringtoUString(const JSValue& value, ExecState* exec)

Is there really a measurable advantage to inlining this? How do I know whether I should call toUString or toUStringInline?
Comment 4 Benjamin Poulain 2012-04-04 22:17:38 PDT
> > Source/JavaScriptCore/ChangeLog:9
> > +        When making operations on primitive types, we loose some time converting
> > +        values to JSString in order to extract the string.
> 
> It looks like JSValue::toString only artificially creates a JSString if the JSValue is a number. I wonder if NumericStrings should just change to cache JSStrings instead of UStrings, and how much of this issue that would resolve.

Yep, it is indeed only for numeric strings.
I can have a look if I can change NumericStrings to use JSString.

> > Source/JavaScriptCore/runtime/JSString.h:472
> > +    ALWAYS_INLINE UString inlineJSValueNotStringtoUString(const JSValue& value, ExecState* exec)
> 
> Is there really a measurable advantage to inlining this? How do I know whether I should call toUString or toUStringInline?

I changed function depending on profiles.

On benchmarks, the gain was fairly big for arrayProtoFuncJoin() and JSArray::sort(). I don't remember the exact numbers but that was > 5% improvement.

I have changed arrayProtoFuncSort() without benchmark because this case seem similar to JSArray::sort().

I think we should always use toUString() unless the profiler shows it is a problem.
Comment 5 Benjamin Poulain 2012-04-05 14:35:21 PDT
(In reply to comment #4)
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        When making operations on primitive types, we loose some time converting
> > > +        values to JSString in order to extract the string.
> > 
> > It looks like JSValue::toString only artificially creates a JSString if the JSValue is a number. I wonder if NumericStrings should just change to cache JSStrings instead of UStrings, and how much of this issue that would resolve.
> 
> Yep, it is indeed only for numeric strings.
> I can have a look if I can change NumericStrings to use JSString.

I looked into changing NumericStrings to use JSString (without implementing anything yet).

I am only half convinced because NumericStrings is also used a lot to create Identifiers, and this would give us only overhead to create JSString in that case.
Also NumericStrings is barely faster than UString::number() and I am afraid of slowing it down further.

I am a bit concerned with using JSString because some of the performance improvements lately were done by avoiding allocating JSString.

Do you know of common cases where we convert Numbers to String and keep the JSString around? This would help me get an idea of the advantage of such change for NumericStrings.
Comment 6 Benjamin Poulain 2012-04-05 17:12:37 PDT
Committed r113396: <http://trac.webkit.org/changeset/113396>