Bug 76592 - [v8]Low efficiency of writing long string in npruntime infrastructure
Summary: [v8]Low efficiency of writing long string in npruntime infrastructure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-18 17:48 PST by Li Yin
Modified: 2012-01-18 23:52 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2012-01-18 21:40 PST, Li Yin
no flags Details | Formatted Diff | Diff
Patch (1.88 KB, patch)
2012-01-18 22:53 PST, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-01-18 17:48:17 PST
[v8]Low efficiency of writing long string in npruntime infrastructure
Comment 1 Li Yin 2012-01-18 21:40:11 PST
Created attachment 123069 [details]
Patch
Comment 2 Li Yin 2012-01-18 21:53:05 PST
In chromium-gtk mode, when the big string was transported from web page to plug-in through invoke function, the elapsed time is too long. I found the root cause was that the time of WriteUtf8 operation was too expensive, when the data type of String was ConsString.

Pseudo-code in JavaScript side:
var str="";
var num=1024*1024;
for(var i=0; i<num; i++)
    str+="A";
plugin_object.writeString(str);

Suppose that plugin_object is a plugin object, and has the function "writeString" in plug-in, although the this function could do nothing.
Comment 3 Li Yin 2012-01-18 21:55:33 PST
Specifying the WriteOptions to HINT_MANY_WRITES_EXPECTED in the calling of WriteUtf8,
the efficiency of WriteUtf8 can be greatly improved. 
On the other hand, it almost has no negative effect for short sting.
Comment 4 Adam Barth 2012-01-18 21:55:47 PST
Comment on attachment 123069 [details]
Patch

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

Looks reasonable.  A couple nits below.  Thanks for the patch!

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * bindings/v8/V8NPUtils.cpp:
> +        (WebCore::convertV8ObjectToNPVariant):

Can you say something about what gets faster and by how much?  That's useful information to have in the ChangeLog.

> Source/WebCore/bindings/v8/V8NPUtils.cpp:69
> +        if (!utf8Chars)
> +            return;

in WebKit, malloc crashes if it can't allocated memory, so this null check isn't needed.

> Source/WebCore/bindings/v8/V8NPUtils.cpp:70
> +        str->WriteUtf8(utf8Chars, length, 0, 1);

Can we use HINT_MANY_WRITES_EXPECTED rather than 1 as the fourth parameter?  As-is it's hard to know what the 1 means.
Comment 5 Li Yin 2012-01-18 22:19:12 PST
1. The efficiency will be improved by 300 times in the best case, when the size of string reaches 1MB.

2. HINT_MANY_WRITES_EXPECTED is internal data type in the class, it can't be accessed in the V8NPUtils.cpp.
Comment 6 Adam Barth 2012-01-18 22:23:04 PST
> 2. HINT_MANY_WRITES_EXPECTED is internal data type in the class, it can't be accessed in the V8NPUtils.cpp.

You can't write v8::String::HINT_MANY_WRITES_EXPECTED ?  It looks like it's declared public in v8.h.
Comment 7 Li Yin 2012-01-18 22:53:36 PST
Created attachment 123076 [details]
Patch
Comment 8 Li Yin 2012-01-18 22:57:15 PST

(In reply to comment #6)
> > 2. HINT_MANY_WRITES_EXPECTED is internal data type in the class, it can't be accessed in the V8NPUtils.cpp.
> 
> You can't write v8::String::HINT_MANY_WRITES_EXPECTED ?  It looks like it's declared public in v8.h.

Yeah, thanks for your reminding and review, and I committed the new patch based on your suggestion, please check it. 
Thanks.
Comment 9 Adam Barth 2012-01-18 22:57:32 PST
Comment on attachment 123076 [details]
Patch

Looks great.  Thanks!
Comment 10 WebKit Review Bot 2012-01-18 23:52:44 PST
Comment on attachment 123076 [details]
Patch

Clearing flags on attachment: 123076

Committed r105389: <http://trac.webkit.org/changeset/105389>
Comment 11 WebKit Review Bot 2012-01-18 23:52:49 PST
All reviewed patches have been landed.  Closing bug.