Summary: | [V8] Replace v8::String::NewSymbol() in CodeGeneratorV8.pm with v8String() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||
Status: | RESOLVED INVALID | ||||||
Severity: | Normal | CC: | abarth, dcarney, japhet, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 93095 | ||||||
Attachments: |
|
Description
Kentaro Hara
2012-08-21 05:02:33 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(). Created attachment 159669 [details]
Patch
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 on attachment 159669 [details]
Patch
OK.
Comment on attachment 159669 [details] Patch Clearing flags on attachment: 159669 Committed r126348: <http://trac.webkit.org/changeset/126348> All reviewed patches have been landed. Closing bug. 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> (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. |