XMLHttpRequest#responseText should use a rope

XMLHttpRequest#responseText should use a rope

Instead of building a flat WTF::String from XMLHttpRequest response data, we
should build a v8::String rope. That way when web sites query the responseText
field repeatedly, every query will share the same underlying data.

This CL causes StringImpl::hashSlowCase to drop way down the CPU profile of
reloading Mobile Gmail:

Before: 1.08%  content_shell  content_shell          [.] WTF::StringImpl::hashSlowCase() const
After:  0.37%  content_shell  content_shell          [.] WTF::StringImpl::hashSlowCase() const

There are likely other savings as well, but the effect on StringImpl::hashSlowCase
is the most dramatic change to the profile.

R=jamesr

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152380

[abarth-chromium, 2013-06-13 05:44:12]

@jamesr: Here's a work-in-progress patch to bring back your original
optimization.  I haven't even tried compiling this yet, but I figured I might as
well upload it.  I got started on this again because I noticed that the
WTF::String -> v8::String conversion show up on a Mobile Gmail CPU profile (in
the guise of StringHash).

[jamesr, 2013-06-13 05:49:36]

nifty!

https://codereview.chromium.org/16925002/diff/1/Source/bindings/v8/ScriptStri...
File Source/bindings/v8/ScriptString.cpp (right):

https://codereview.chromium.org/16925002/diff/1/Source/bindings/v8/ScriptStri...
Source/bindings/v8/ScriptString.cpp:43: return
ScriptString(v8::String::Concat(a, ));
(a, b), i presume? :)

[abarth-chromium, 2013-06-13 05:56:12]

https://codereview.chromium.org/16925002/diff/1/Source/bindings/v8/ScriptStri...
File Source/bindings/v8/ScriptString.cpp (right):

https://codereview.chromium.org/16925002/diff/1/Source/bindings/v8/ScriptStri...
Source/bindings/v8/ScriptString.cpp:43: return
ScriptString(v8::String::Concat(a, ));
On 2013/06/13 05:49:36, jamesr wrote:
> (a, b), i presume? :)

Yes.  As I said, I haven't tried compiling it.  :)

Another approach is to have a WTF::Rope data structure, and then efficiently
convert it to a v8::String using Concat.  That wouldn't be quite as efficient
though.  We'd end up sharing the underlying string buffers, but the graph
superstructure would be created separately each time we converted to v8::String.

[jamesr, 2013-06-13 05:59:09]

That would require caching the V8 handles for each conversion, right? Otherwise
we'd have a whole bunch of v8 handles pointed at the same underlying strings,
which seems like a bummer.

[abarth-chromium, 2013-06-13 17:08:36]

On 2013/06/13 05:59:09, jamesr wrote:
> That would require caching the V8 handles for each conversion, right?
Otherwise
> we'd have a whole bunch of v8 handles pointed at the same underlying strings,
> which seems like a bummer.

Correct.  However, we already do that.  :)

[abarth-chromium, 2013-06-13 18:47:27]

@jamesr: This CL is ready for review now.

[jamesr, 2013-06-13 19:28:42]

lgtm

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptString.h (right):

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptString.h:43: ScriptString(v8::Handle<v8::String> value)
: ScriptValue(value) { }
probably want explicit here, no?

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptString.h:46: String toString() const;
i think this could use a comment indicating that it's a relatively expensive
operation

https://codereview.chromium.org/16925002/diff/13001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorInstrumentation.h (right):

https://codereview.chromium.org/16925002/diff/13001/Source/core/inspector/Ins...
Source/core/inspector/InspectorInstrumentation.h:35: #include
"bindings/v8/ScriptString.h"
is this used?

[abarth-chromium, 2013-06-13 19:38:53]

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptString.h (right):

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptString.h:43: ScriptString(v8::Handle<v8::String> value)
: ScriptValue(value) { }
On 2013/06/13 19:28:42, jamesr wrote:
> probably want explicit here, no?

Yeah.

https://codereview.chromium.org/16925002/diff/13001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptString.h:46: String toString() const;
On 2013/06/13 19:28:42, jamesr wrote:
> i think this could use a comment indicating that it's a relatively expensive
> operation

Maybe we should call it "flattenToString" ?

https://codereview.chromium.org/16925002/diff/13001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorInstrumentation.h (right):

https://codereview.chromium.org/16925002/diff/13001/Source/core/inspector/Ins...
Source/core/inspector/InspectorInstrumentation.h:35: #include
"bindings/v8/ScriptString.h"
On 2013/06/13 19:28:42, jamesr wrote:
> is this used?

Yeah, it's used in the InspectorInstrumentation.idl, which uses this header to
get its includes.

[commit-bot: I haz the power, 2013-06-13 19:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/16925002/19001

[commit-bot: I haz the power, 2013-06-13 21:19:42]

Change committed as 152380