Rewrite decodeURL as builtin function, remove now unused runtime functions.

[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org

Committed: https://crrev.com/8c31bd81f2919aafb4d586f39aa729cdbff23c44
Cr-Commit-Position: refs/heads/master@{#36543}

[Franzi, 2016-05-19 09:18:08]

Description was changed from

==========
Rewrite decodeURL as builtin function, remove now unused runtime functions.

BUG=v8:4912
==========

to

==========
[builtins] Rewrite decodeURI as builtin function.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
- TruncateString
- NewString
- OneByteSeqStringGetChar
- OneByteSeqStringSetChar
- TwoByteSeqStringGetChar
- TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with decoding large strings, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912
R=yangguo@chromium.org, bmeurer@chromium.org
==========

[Franzi, 2016-05-19 11:16:24]

Description was changed from

==========
[builtins] Rewrite decodeURI as builtin function.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
- TruncateString
- NewString
- OneByteSeqStringGetChar
- OneByteSeqStringSetChar
- TwoByteSeqStringGetChar
- TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with decoding large strings, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912
R=yangguo@chromium.org, bmeurer@chromium.org
==========

to

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912
R=yangguo@chromium.org, bmeurer@chromium.org
==========

[Franzi, 2016-05-20 10:39:22]

Patchset #6 (id:100001) has been deleted

[Franzi, 2016-05-20 10:39:35]

Patchset #5 (id:80001) has been deleted

[Franzi, 2016-05-20 12:30:40]

franzih@chromium.org changed reviewers:
+ bmeurer@chromium.org, yangguo@chromium.org

[Franzi, 2016-05-20 12:30:42]

Hi Yang, Hi Benedikt

here's the second part of 4912, URL encoding rewritten as builtin functions. 
Could you have a look please.

Thanks a lot, 

Franzi

PS: I was trying to use unibrow::Utf8::Encode() and ValueOf(), but couldn't get
it to work: kBadChar (65535) is returned from ValueOf() to indicate failure, but
it is also the decoded value of valid input %EF%BF%BD. I double check if I can
file an issue for that and work on it separately.

[Franzi, 2016-05-20 17:47:19]

I simplified the octed decoding/encoding some more.

[Benedikt Meurer, 2016-05-21 19:26:12]

Description was changed from

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912
R=yangguo@chromium.org, bmeurer@chromium.org
==========

to

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org
==========

[Benedikt Meurer, 2016-05-21 19:28:00]

Looking good, thanks, I like the awesome code removal (wanna kill the rest of
uri.js in a follow up CL? ;-))!

One main beef for me is the use of naked Object pointers; I'd rather go for
MaybeHandle<Object> instead.

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc#newcod...
src/builtins.cc:2109: Handle<String> encodedURI;
Nit: Please rename to encoded_uri.

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc#newcod...
src/builtins.cc:2119: Handle<String> encodedURIComponent;
Nit: Please rename to encoded_uri_component.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h
File src/uri.h (right):

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode14
src/uri.h:14: class Uri : public AllStatic {
I'd rather handlify these functions completely. Returning naked Object pointers
was a major source of bugs in the past (sorry, didn't comment on the CL that
introduced Uri the week before).

So if this is OK with Yang, then please change these functions to return
MaybeHandle<Object> instead of Object*.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode25
src/uri.h:25: static Object* DecodeUri(Isolate* isolate, Handle<String> uri) {
Nit: Please add references to the EcmaScript standard.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode29
src/uri.h:29: static Object* DecodeUriComponent(Isolate* isolate,
Nit: Please add references to the EcmaScript standard.

[Franzi, 2016-05-22 12:34:08]

Hi Benedikt, 

Thanks for looking at this. I implemented your suggestions. Removing the rest of
uri.js in another CL was the plan :)

Cheers, 

Franzi

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc#newcod...
src/builtins.cc:2109: Handle<String> encodedURI;
On 2016/05/21 19:27:59, Benedikt Meurer wrote:
> Nit: Please rename to encoded_uri.

Done.

https://codereview.chromium.org/1994733003/diff/160001/src/builtins.cc#newcod...
src/builtins.cc:2119: Handle<String> encodedURIComponent;
On 2016/05/21 19:27:59, Benedikt Meurer wrote:
> Nit: Please rename to encoded_uri_component.

Done. Changed it in encodeURIComponent as well.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h
File src/uri.h (right):

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode14
src/uri.h:14: class Uri : public AllStatic {
On 2016/05/21 19:27:59, Benedikt Meurer wrote:
> I'd rather handlify these functions completely. Returning naked Object
pointers
> was a major source of bugs in the past (sorry, didn't comment on the CL that
> introduced Uri the week before).
> 
> So if this is OK with Yang, then please change these functions to return
> MaybeHandle<Object> instead of Object*.

Done.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode25
src/uri.h:25: static Object* DecodeUri(Isolate* isolate, Handle<String> uri) {
On 2016/05/21 19:27:59, Benedikt Meurer wrote:
> Nit: Please add references to the EcmaScript standard.

Done.

https://codereview.chromium.org/1994733003/diff/160001/src/uri.h#newcode29
src/uri.h:29: static Object* DecodeUriComponent(Isolate* isolate,
On 2016/05/21 19:27:59, Benedikt Meurer wrote:
> Nit: Please add references to the EcmaScript standard.

Done.

[Benedikt Meurer, 2016-05-22 17:12:44]

Thanks. LGTM from my side.

[Yang, 2016-05-23 06:44:32]

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc
File src/uri.cc (right):

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode148
src/uri.cc:148: bool IsRepalcementCharacter(List<uint8_t>* octets) {
typo.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode149
src/uri.cc:149: // 0xFFFD is %ef%bf%bd
What does this comment mean?

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode176
src/uri.cc:176: bool TwoDigitHex(uc16& decoded, int k, String::FlatContent*
uri_content) {
Can we use uc16* as argument type? That way it's easier to understand, from the
call site, that the argument is an output.

Also see https://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode176
src/uri.cc:176: bool TwoDigitHex(uc16& decoded, int k, String::FlatContent*
uri_content) {
can we call the second argument "index" or something instead of "k"?

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode177
src/uri.cc:177: char high = HexValue(uri_content->Get(k + 1));
FlatContent::Get returns a uc16. Casting that to signed char and then comparing
that to 0 smells like undefined behavior.

Let's store the result as uc16 and compare to Utf8::kMaxOneByteChar.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode187
src/uri.cc:187: void AddToBuffer(uc16 decoded, String::FlatContent* uri_content,
int k,
same here, "index" instead of "k".

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode192
src/uri.cc:192: buffer->Add(uri_content->Get(k + 2));
Can we have a safeguard here that we don't have implicit clamping here? I.e.
that the return value uri_content->Get fits into the buffer type T?

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode211
src/uri.cc:211: while (((decoded << ++n) & 0x80) != 0) {
Can we have this as 

do {
  n++;
} while ((decoded << n) & 0x80);

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode216
src/uri.cc:216: List<uint8_t> octets;
octets will at most have the length 4, right? Can we have a simple array and
pass the length to DecodeOctets?

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode296
src/uri.cc:296: isolate, result, isolate->factory()->NewConsString(left, right),
Object);
Since we are going to copy from list into the heap anyways, can we simply, for
the mixed encoding case, allocate a large raw two-byte string and CopyChar both
lists into it?

https://codereview.chromium.org/1994733003/diff/180001/test/mjsunit/regress-6...
File test/mjsunit/regress-612109.js (right):

https://codereview.chromium.org/1994733003/diff/180001/test/mjsunit/regress-6...
test/mjsunit/regress-612109.js:8: decodeURI(encodeURI(__v_2));
Can we rename this file to regress-crbug-612109? And maybe fix the variable
names.

[Franzi, 2016-05-23 08:55:57]

Thanks for the review, I addressed your comments except for the one about
src/uri.cc line 177. 

Could you have another look. Thanks, 

Franzi

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc
File src/uri.cc (right):

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode148
src/uri.cc:148: bool IsRepalcementCharacter(List<uint8_t>* octets) {
On 2016/05/23 06:44:32, Yang wrote:
> typo.

Done.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode149
src/uri.cc:149: // 0xFFFD is %ef%bf%bd
On 2016/05/23 06:44:32, Yang wrote:
> What does this comment mean?

Reworded the comment to clarify why we check for this particular encoding.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode176
src/uri.cc:176: bool TwoDigitHex(uc16& decoded, int k, String::FlatContent*
uri_content) {
On 2016/05/23 06:44:32, Yang wrote:
> can we call the second argument "index" or something instead of "k"?

Done.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode176
src/uri.cc:176: bool TwoDigitHex(uc16& decoded, int k, String::FlatContent*
uri_content) {
On 2016/05/23 06:44:31, Yang wrote:
> Can we use uc16* as argument type? That way it's easier to understand, from
the
> call site, that the argument is an output.
> 
> Also see https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Done.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode177
src/uri.cc:177: char high = HexValue(uri_content->Get(k + 1));
On 2016/05/23 06:44:32, Yang wrote:
> FlatContent::Get returns a uc16. Casting that to signed char and then
comparing
> that to 0 smells like undefined behavior.
> 
> Let's store the result as uc16 and compare to Utf8::kMaxOneByteChar.

Not sure I understand the comment. HexValue takes a uc32 and returns an int
between -1 and 16. Is that not OK to store as a signed char?

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode187
src/uri.cc:187: void AddToBuffer(uc16 decoded, String::FlatContent* uri_content,
int k,
On 2016/05/23 06:44:32, Yang wrote:
> same here, "index" instead of "k".

Done.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode192
src/uri.cc:192: buffer->Add(uri_content->Get(k + 2));
On 2016/05/23 06:44:31, Yang wrote:
> Can we have a safeguard here that we don't have implicit clamping here? I.e.
> that the return value uri_content->Get fits into the buffer type T?

Done. Throwing exception if uri_content->Get() is larger than
numeric_limits::max() for type T.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode211
src/uri.cc:211: while (((decoded << ++n) & 0x80) != 0) {
On 2016/05/23 06:44:31, Yang wrote:
> Can we have this as 
> 
> do {
>   n++;
> } while ((decoded << n) & 0x80);

Changed it to a simple while loop:

    int n = 1;
        while((decoded << n) & 0x80) {
          n++;
        }

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode216
src/uri.cc:216: List<uint8_t> octets;
On 2016/05/23 06:44:31, Yang wrote:
> octets will at most have the length 4, right? Can we have a simple array and
> pass the length to DecodeOctets?

Done.

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode296
src/uri.cc:296: isolate, result, isolate->factory()->NewConsString(left, right),
Object);
On 2016/05/23 06:44:32, Yang wrote:
> Since we are going to copy from list into the heap anyways, can we simply, for
> the mixed encoding case, allocate a large raw two-byte string and CopyChar
both
> lists into it?

Returning sequential one- or two-byte string.

https://codereview.chromium.org/1994733003/diff/180001/test/mjsunit/regress-6...
File test/mjsunit/regress-612109.js (right):

https://codereview.chromium.org/1994733003/diff/180001/test/mjsunit/regress-6...
test/mjsunit/regress-612109.js:8: decodeURI(encodeURI(__v_2));
On 2016/05/23 06:44:32, Yang wrote:
> Can we rename this file to regress-crbug-612109? And maybe fix the variable
> names.

Done.

[Yang, 2016-05-23 11:24:59]

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc
File src/uri.cc (right):

https://codereview.chromium.org/1994733003/diff/180001/src/uri.cc#newcode177
src/uri.cc:177: char high = HexValue(uri_content->Get(k + 1));
On 2016/05/23 08:55:57, Franzi wrote:
> On 2016/05/23 06:44:32, Yang wrote:
> > FlatContent::Get returns a uc16. Casting that to signed char and then
> comparing
> > that to 0 smells like undefined behavior.
> > 
> > Let's store the result as uc16 and compare to Utf8::kMaxOneByteChar.
> 
> Not sure I understand the comment. HexValue takes a uc32 and returns an int
> between -1 and 16. Is that not OK to store as a signed char?

Ah I see. I misunderstood. Nevermind this comment.

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc
File src/uri.cc (right):

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode193
src/uri.cc:193: if (first > std::numeric_limits<T>::max() ||
Do you expect this to happen in some cases? Imo this must not happen at all,
since you should have figured out the correct encoding in IntoOneAndTwoByte.
Please make this a DCHECK.

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode218
src/uri.cc:218: while ((decoded << n) & 0x80) {
Actually, you could merge this while loop with the for loop below, right? I
wonder whether there is performance impact through this.

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode307
src/uri.cc:307:
isolate->factory()->NewRawOneByteString(one_byte_buffer.length()),
You can use NewStringFromOneByte here.

[Franzi, 2016-05-24 15:07:38]

Hi Yang, 

I addressed your comments and rebased. Could you have another look. 

Thanks, 

Franzi

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc
File src/uri.cc (right):

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode193
src/uri.cc:193: if (first > std::numeric_limits<T>::max() ||
On 2016/05/23 11:24:59, Yang wrote:
> Do you expect this to happen in some cases? Imo this must not happen at all,
> since you should have figured out the correct encoding in IntoOneAndTwoByte.
> Please make this a DCHECK.
You're right. Can never happen. Done.

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode218
src/uri.cc:218: while ((decoded << n) & 0x80) {
On 2016/05/23 11:24:59, Yang wrote:
> Actually, you could merge this while loop with the for loop below, right? I
> wonder whether there is performance impact through this.

Merged. Also renamed n to number_of_continuation_bytes and removed double checks
(n==1 and k+2>length)

https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode307
src/uri.cc:307:
isolate->factory()->NewRawOneByteString(one_byte_buffer.length()),
On 2016/05/23 11:24:59, Yang wrote:
> You can use NewStringFromOneByte here.

Done.

[Yang, 2016-05-27 08:24:34]

On 2016/05/24 15:07:38, Franzi wrote:
> Hi Yang, 
> 
> I addressed your comments and rebased. Could you have another look. 
> 
> Thanks, 
> 
> Franzi
> 
> https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc
> File src/uri.cc (right):
> 
> https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode193
> src/uri.cc:193: if (first > std::numeric_limits<T>::max() ||
> On 2016/05/23 11:24:59, Yang wrote:
> > Do you expect this to happen in some cases? Imo this must not happen at all,
> > since you should have figured out the correct encoding in IntoOneAndTwoByte.
> > Please make this a DCHECK.
> You're right. Can never happen. Done.
> 
> https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode218
> src/uri.cc:218: while ((decoded << n) & 0x80) {
> On 2016/05/23 11:24:59, Yang wrote:
> > Actually, you could merge this while loop with the for loop below, right? I
> > wonder whether there is performance impact through this.
> 
> Merged. Also renamed n to number_of_continuation_bytes and removed double
checks
> (n==1 and k+2>length)
> 
> https://codereview.chromium.org/1994733003/diff/220001/src/uri.cc#newcode307
> src/uri.cc:307:
> isolate->factory()->NewRawOneByteString(one_byte_buffer.length()),
> On 2016/05/23 11:24:59, Yang wrote:
> > You can use NewStringFromOneByte here.
> 
> Done.

lgtm

[Franzi, 2016-05-27 08:31:03]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-05-27 08:31:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1994733003/#ps280001
(title: "Change order of decode/encode")

[commit-bot: I haz the power, 2016-05-27 08:31:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994733003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994733003/280001

[commit-bot: I haz the power, 2016-05-27 08:59:19]

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

[commit-bot: I haz the power, 2016-05-27 08:59:20]

Failed to apply patch for src/crankshaft/hydrogen.h:
While running git apply --index -3 -p1;
  error: patch failed: src/crankshaft/hydrogen.h:2265
  Falling back to three-way merge...
  Applied patch to 'src/crankshaft/hydrogen.h' with conflicts.
  U src/crankshaft/hydrogen.h

Patch:       src/crankshaft/hydrogen.h
Index: src/crankshaft/hydrogen.h
diff --git a/src/crankshaft/hydrogen.h b/src/crankshaft/hydrogen.h
index
282b43866edc9d60ac142bf103b169a4cdf0884f..29be654086fa3d956172661f5e91d100c73f1c6d
100644
--- a/src/crankshaft/hydrogen.h
+++ b/src/crankshaft/hydrogen.h
@@ -2265,8 +2265,6 @@ class HOptimizedGraphBuilder : public HGraphBuilder,
public AstVisitor {
   F(ValueOf)                           \
   F(StringCharFromCode)                \
   F(StringCharAt)                      \
-  F(OneByteSeqStringSetChar)           \
-  F(TwoByteSeqStringSetChar)           \
   F(ToInteger)                         \
   F(ToName)                            \
   F(ToObject)                          \

[Franzi, 2016-05-27 09:26:55]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-05-27 09:26:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1994733003/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-27 09:27:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994733003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994733003/300001

[commit-bot: I haz the power, 2016-05-27 09:56:13]

Description was changed from

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org
==========

to

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org
==========

[commit-bot: I haz the power, 2016-05-27 09:56:15]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2016-05-27 09:57:18]

Description was changed from

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org
==========

to

==========
[builtins] Rewrite uri.js as builtin functions.

Rewrite decodeURI and decodeURIComponent as builtin functions
and install them in the bootstrapper.

Delete unused runtime functions:
 - TruncateString
 - NewString
 - OneByteSeqStringGetChar
 - OneByteSeqStringSetChar
 - TwoByteSeqStringGetChar
 - TwoByteSeqStringSetChar

Add regression test for decoding large strings. Clusterfuzz detected
a problem with %TruncateString, see
https://bugs.chromium.org/p/chromium/issues/detail?id=612109#c6
This is automatically fixed by this rewrite because %TruncateString
is deleted anyways.

Crude benchmark on 585 decodeURI and decodeURIComponent tests
averaged over five runs:

* builtin functions
real	0m9.69s
user	2m39.8816s
sys    	0m12.6398s

* JS functions calling into the runtime e.g., for %TruncateString
real	0m11.0598s
user	3m6.7026s
sys	0m13.5756s

By running:
$  time tools/run-tests.py   --arch=x64  --mode=Release --buildbot
  test262/built-ins/decodeURI* mjsunit/uri
>>> Running tests for x64.Release

BUG=v8:4912, chromium:612109
R=yangguo@chromium.org, bmeurer@chromium.org

Committed: https://crrev.com/8c31bd81f2919aafb4d586f39aa729cdbff23c44
Cr-Commit-Position: refs/heads/master@{#36543}
==========

[commit-bot: I haz the power, 2016-05-27 09:57:20]

Patchset 14 (id:??) landed as
https://crrev.com/8c31bd81f2919aafb4d586f39aa729cdbff23c44
Cr-Commit-Position: refs/heads/master@{#36543}