Bug 94588 - [V8] Replace v8::String::NewSymbol() in CodeGeneratorV8.pm with v8String()
Summary: [V8] Replace v8::String::NewSymbol() in CodeGeneratorV8.pm with v8String()
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 93095
  Show dependency treegraph
 
Reported: 2012-08-21 05:02 PDT by Kentaro Hara
Modified: 2012-08-22 23:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (27.83 KB, patch)
2012-08-21 05:51 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-08-21 05:02:33 PDT
v8String() is much faster than v8::String::NewSymbol().

// 83 nano sec
static v8::Handle<v8::Value> attr1AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    static String str1 = String("foo");
    static String str2 = String("foo");
    v8::Handle<v8::Value> v1 = v8String(str1);
    v8::Handle<v8::Value> v2 = v8String(str2);
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}

// 86 nano sec
static v8::Handle<v8::Value> attr2AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    static String str1 = String("foo");
    static String str2 = String("bar");
    v8::Handle<v8::Value> v1 = v8String(str1);
    v8::Handle<v8::Value> v2 = v8String(str2);
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}

// 376 nano sec
static v8::Handle<v8::Value> attr3AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    v8::Handle<v8::Value> v1 = v8::String::NewSymbol("foo", 3);
    v8::Handle<v8::Value> v2 = v8::String::NewSymbol("foo", 3);
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}

// 377 nano sec
static v8::Handle<v8::Value> attr4AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    v8::Handle<v8::Value> v1 = v8::String::NewSymbol("foo", 3);
    v8::Handle<v8::Value> v2 = v8::String::NewSymbol("bar", 3);
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}
Comment 1 Kentaro Hara 2012-08-21 05:40:37 PDT
Sorry, the above performance measurement was not fair. We need to compare the time for converting char* to v8::String.

Updated result:

// 272 nano sec
static v8::Handle<v8::Value> attr3AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    v8::Handle<v8::Value> v1 = v8String(String("foo"));
    v8::Handle<v8::Value> v2 = v8String(String("bar"));
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}

// 377 nano sec
static v8::Handle<v8::Value> attr4AttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    v8::Handle<v8::Value> v1 = v8::String::NewSymbol("foo");
    v8::Handle<v8::Value> v2 = v8::String::NewSymbol("bar");
    if (!v1.IsEmpty() && !v2.IsEmpty())
        return v8Undefined();
    return v8::Null(); // Never reach.
}

Anyway, v8String() is faster than String::NewSymbol().
Comment 2 Kentaro Hara 2012-08-21 05:51:54 PDT
Created attachment 159669 [details]
Patch
Comment 3 Adam Barth 2012-08-21 08:16:00 PDT
Do you have any thoughts on https://bugs.webkit.org/show_bug.cgi?id=94574#c5 ?  I wonder if there's a place for NewSymbol in some cases.
Comment 4 Eric Seidel (no email) 2012-08-22 13:31:15 PDT
Comment on attachment 159669 [details]
Patch

OK.
Comment 5 WebKit Review Bot 2012-08-22 14:13:49 PDT
Comment on attachment 159669 [details]
Patch

Clearing flags on attachment: 159669

Committed r126348: <http://trac.webkit.org/changeset/126348>
Comment 6 WebKit Review Bot 2012-08-22 14:13:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Kentaro Hara 2012-08-22 23:54:18 PDT
Reverted r126348 for reason:

We should use v8::String::NewSymbol() for symbols. See https://bugs.webkit.org/show_bug.cgi?id=94574#c10 for more details

Committed r126397: <http://trac.webkit.org/changeset/126397>
Comment 8 Kentaro Hara 2012-08-22 23:55:36 PDT
(In reply to comment #6)
> All reviewed patches have been landed.  Closing bug.

This patch is not quite right. As abarth pointed out, we should use String::NewSymbol() for symbols and use String::New() for other strings. See https://bugs.webkit.org/show_bug.cgi?id=94574#c10 for more details.