Use StringView for v8String().

Use StringView for v8String().

By using StringView here we can avoid allocating externalized strings
for all of the literal strings used like v8String(isolate, "example")
where we would first allocate a WTF::String then externalize it into
a v8::String. Instead we can just go directly to a v8::String which is
faster and uses less memory.

BUG=615174
Committed: https://crrev.com/d4a17e1863ab232b718caeb4afdd4252af99cef0
Cr-Commit-Position: refs/heads/master@{#414920}

[esprehn, 2016-08-27 00:01:44]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 00:03:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-08-27 00:57:58]

Description was changed from

==========
Use StringView for v8String().

BUG=
==========

to

==========
Use StringView for v8String().

By using StringView here we can avoid allocating externalized strings
for all of the literal strings used like v8String(isolate, "example")
where we would first allocate a WTF::String then externalize it into
a v8::String. Instead we can just go directly to a v8::String which is
faster and uses less memory.

BUG=615174
==========

[esprehn, 2016-08-27 01:16:39]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org, yutak@chromium.org

[haraken, 2016-08-27 01:21:46]

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return
v8::String::NewFromOneByte(isolate, reinterpret_cast<const
uint8_t*>(string.characters8()), v8::NewStringType::kNormal,
static_cast<int>(string.length())).ToLocalChecked();

Don't you want to use:

  V8PerIsolateData::from(isolate)->getStringCache()->v8ExternalString(isolate,
string.impl());

?

If we're highly likely to hit the branch at line 406, I'm fine with not using a
StringCache here though.

[commit-bot: I haz the power, 2016-08-27 02:09:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-27 02:09:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[esprehn, 2016-08-27 02:18:57]

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return
v8::String::NewFromOneByte(isolate, reinterpret_cast<const
uint8_t*>(string.characters8()), v8::NewStringType::kNormal,
static_cast<int>(string.length())).ToLocalChecked();
On 2016/08/27 at 01:21:46, haraken wrote:
> Don't you want to use:
> 
>   V8PerIsolateData::from(isolate)->getStringCache()->v8ExternalString(isolate,
string.impl());
> 
> ?

Hmm? There's no such thing as string.impl() on a StringView, just sharedImpl()
which is already above.

> 
> If we're highly likely to hit the branch at line 406, I'm fine with not using
a StringCache here though.

We hit that branch for things like toV8 in a bunch of cases, we hit the NewFrom*
cases for literal strings.

StringView makes it so that if you pass an AtomicString/String we'll
externalize, but if you pass a literal string we'll just allocate a string and
not create a WTF string + StringResource and then externalize which is much more
expensive.

[haraken, 2016-08-27 02:21:52]

On 2016/08/27 02:18:57, esprehn wrote:
>
https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return
> v8::String::NewFromOneByte(isolate, reinterpret_cast<const
> uint8_t*>(string.characters8()), v8::NewStringType::kNormal,
> static_cast<int>(string.length())).ToLocalChecked();
> On 2016/08/27 at 01:21:46, haraken wrote:
> > Don't you want to use:
> > 
> >  
V8PerIsolateData::from(isolate)->getStringCache()->v8ExternalString(isolate,
> string.impl());
> > 
> > ?
> 
> Hmm? There's no such thing as string.impl() on a StringView, just sharedImpl()
> which is already above.
> 
> > 
> > If we're highly likely to hit the branch at line 406, I'm fine with not
using
> a StringCache here though.
> 
> We hit that branch for things like toV8 in a bunch of cases, we hit the
NewFrom*
> cases for literal strings.
> 
> StringView makes it so that if you pass an AtomicString/String we'll
> externalize, but if you pass a literal string we'll just allocate a string and
> not create a WTF string + StringResource and then externalize which is much
more
> expensive.

Ah, I understand your point. LGTM.

[esprehn, 2016-08-27 02:24:03]

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return
v8::String::NewFromOneByte(isolate, reinterpret_cast<const
uint8_t*>(string.characters8()), v8::NewStringType::kNormal,
static_cast<int>(string.length())).ToLocalChecked();
ex. 

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

that hits the sharedImpl() case which will then externalize the string using the
cache.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

that will return null from sharedImpl() and so it'll go down the NewFromOneByte
path which is better since we don't waste time externalizing the temporary
string.

[esprehn, 2016-08-27 02:24:47]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-08-27 02:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-08-27 08:26:18]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2016-08-27 08:26:21]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-08-27 08:26:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 11:05:25]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-27 11:08:39]

Description was changed from

==========
Use StringView for v8String().

By using StringView here we can avoid allocating externalized strings
for all of the literal strings used like v8String(isolate, "example")
where we would first allocate a WTF::String then externalize it into
a v8::String. Instead we can just go directly to a v8::String which is
faster and uses less memory.

BUG=615174
==========

to

==========
Use StringView for v8String().

By using StringView here we can avoid allocating externalized strings
for all of the literal strings used like v8String(isolate, "example")
where we would first allocate a WTF::String then externalize it into
a v8::String. Instead we can just go directly to a v8::String which is
faster and uses less memory.

BUG=615174

Committed: https://crrev.com/d4a17e1863ab232b718caeb4afdd4252af99cef0
Cr-Commit-Position: refs/heads/master@{#414920}
==========

[commit-bot: I haz the power, 2016-08-27 11:08:39]

Patchset 1 (id:??) landed as
https://crrev.com/d4a17e1863ab232b718caeb4afdd4252af99cef0
Cr-Commit-Position: refs/heads/master@{#414920}