Bug 83243

Summary: Speed up the conversion from JSValue to String for bulk operations
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: JavaScriptCoreAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 83180    
Attachments:
Description Flags
Patch ggaren: review+

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>