math.js: Use %_TypedArrayGetLength to get length

math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray
length property writable, which means we shouldn't depend on it.
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776
Committed: https://crrev.com/a7d091ffe385fc62e372f34317d97e6ea31a0a4a
Cr-Commit-Position: refs/heads/master@{#36655}

[gsathya, 2016-06-01 01:44:48]

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

[commit-bot: I haz the power, 2016-06-01 01:44:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020203006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2020203006/20001

[commit-bot: I haz the power, 2016-06-01 02:08:39]

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

[commit-bot: I haz the power, 2016-06-01 02:08:40]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-06-01 03:05:46]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray length
property writable, which means we shouldn't depend on it. Instead, use
the %_TypedArrayGetLength% call to get length.

Attached regression test.

BUG=v8:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray length
property writable, which means we shouldn't depend on it. Instead, use
the %_TypedArrayGetLength% call to get length.

Attached regression test.

BUG=v8:615776
==========

[gsathya, 2016-06-01 03:05:47]

gsathya@chromium.org changed reviewers:
+ yangguo@chromium.org

[gsathya, 2016-06-01 03:06:11]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray length
property writable, which means we shouldn't depend on it. Instead, use
the %_TypedArrayGetLength% call to get length.

Attached regression test.

BUG=v8:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray length
property writable, which means we shouldn't depend on it. Instead, use
%_TypedArrayGetLength% to get length.

Attached regression test.

BUG=v8:615776
==========

[gsathya, 2016-06-01 03:06:26]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray length
property writable, which means we shouldn't depend on it. Instead, use
%_TypedArrayGetLength% to get length.

Attached regression test.

BUG=v8:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=v8:615776
==========

[gsathya, 2016-06-01 03:07:10]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=v8:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776
==========

[Yang, 2016-06-01 03:29:49]

On 2016/06/01 03:06:34, gsathya wrote:

can you reduce the test case a bit? The current one only fails because WeakMap
happens to be based on Math.random internally.

[Yang, 2016-06-01 03:29:51]

On 2016/06/01 03:06:34, gsathya wrote:

can you reduce the test case a bit? The current one only fails because WeakMap
happens to be based on Math.random internally.

[gsathya, 2016-06-01 03:41:58]

On 2016/06/01 03:29:51, Yang wrote:
> On 2016/06/01 03:06:34, gsathya wrote:
> 
> can you reduce the test case a bit? The current one only fails because WeakMap
> happens to be based on Math.random internally.

I ended up removing the second test case (since they both fail because of the
same thing). Let me know if you'd rather have a trimmed version of the second
test case like in patchset3
(https://codereview.chromium.org/2020203006/#ps40001)

[Yang, 2016-06-01 03:45:02]

On 2016/06/01 03:41:58, gsathya wrote:
> On 2016/06/01 03:29:51, Yang wrote:
> > On 2016/06/01 03:06:34, gsathya wrote:
> > 
> > can you reduce the test case a bit? The current one only fails because
WeakMap
> > happens to be based on Math.random internally.
> 
> I ended up removing the second test case (since they both fail because of the
> same thing). Let me know if you'd rather have a trimmed version of the second
> test case like in patchset3
> (https://codereview.chromium.org/2020203006/#ps40001)

lgtm. What I had in mind was to call Math.random directly.

[Yang, 2016-06-01 03:45:03]

On 2016/06/01 03:41:58, gsathya wrote:
> On 2016/06/01 03:29:51, Yang wrote:
> > On 2016/06/01 03:06:34, gsathya wrote:
> > 
> > can you reduce the test case a bit? The current one only fails because
WeakMap
> > happens to be based on Math.random internally.
> 
> I ended up removing the second test case (since they both fail because of the
> same thing). Let me know if you'd rather have a trimmed version of the second
> test case like in patchset3
> (https://codereview.chromium.org/2020203006/#ps40001)

lgtm. What I had in mind was to call Math.random directly.

[gsathya, 2016-06-01 04:27:08]

On 2016/06/01 03:45:03, Yang wrote:
> On 2016/06/01 03:41:58, gsathya wrote:
> > On 2016/06/01 03:29:51, Yang wrote:
> > > On 2016/06/01 03:06:34, gsathya wrote:
> > > 
> > > can you reduce the test case a bit? The current one only fails because
> WeakMap
> > > happens to be based on Math.random internally.
> > 
> > I ended up removing the second test case (since they both fail because of
the
> > same thing). Let me know if you'd rather have a trimmed version of the
second
> > test case like in patchset3
> > (https://codereview.chromium.org/2020203006/#ps40001)
> 
> lgtm. What I had in mind was to call Math.random directly.

Done. PTAL.

[Yang, 2016-06-01 04:33:30]

On 2016/06/01 04:27:08, gsathya wrote:
> On 2016/06/01 03:45:03, Yang wrote:
> > On 2016/06/01 03:41:58, gsathya wrote:
> > > On 2016/06/01 03:29:51, Yang wrote:
> > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > 
> > > > can you reduce the test case a bit? The current one only fails because
> > WeakMap
> > > > happens to be based on Math.random internally.
> > > 
> > > I ended up removing the second test case (since they both fail because of
> the
> > > same thing). Let me know if you'd rather have a trimmed version of the
> second
> > > test case like in patchset3
> > > (https://codereview.chromium.org/2020203006/#ps40001)
> > 
> > lgtm. What I had in mind was to call Math.random directly.
> 
> Done. PTAL.

lgtm.

however, this still requires MathRandom to not guard against zero-length typed
array. I could imagine that, were it implemented slightly differently, it might
simply carry on.

We could install a getter for the length property that then either throws, or
calls %Abort.

[gsathya, 2016-06-01 04:43:34]

On 2016/06/01 04:33:30, Yang wrote:
> On 2016/06/01 04:27:08, gsathya wrote:
> > On 2016/06/01 03:45:03, Yang wrote:
> > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > 
> > > > > can you reduce the test case a bit? The current one only fails because
> > > WeakMap
> > > > > happens to be based on Math.random internally.
> > > > 
> > > > I ended up removing the second test case (since they both fail because
of
> > the
> > > > same thing). Let me know if you'd rather have a trimmed version of the
> > second
> > > > test case like in patchset3
> > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > 
> > > lgtm. What I had in mind was to call Math.random directly.
> > 
> > Done. PTAL.
> 
> lgtm.
> 
> however, this still requires MathRandom to not guard against zero-length typed
> array. I could imagine that, were it implemented slightly differently, it
might
> simply carry on.
> 
> We could install a getter for the length property that then either throws, or
> calls %Abort.

Fair enough. Updated.

[Yang, 2016-06-01 04:46:06]

On 2016/06/01 04:43:34, gsathya wrote:
> On 2016/06/01 04:33:30, Yang wrote:
> > On 2016/06/01 04:27:08, gsathya wrote:
> > > On 2016/06/01 03:45:03, Yang wrote:
> > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > 
> > > > > > can you reduce the test case a bit? The current one only fails
because
> > > > WeakMap
> > > > > > happens to be based on Math.random internally.
> > > > > 
> > > > > I ended up removing the second test case (since they both fail because
> of
> > > the
> > > > > same thing). Let me know if you'd rather have a trimmed version of the
> > > second
> > > > > test case like in patchset3
> > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > 
> > > > lgtm. What I had in mind was to call Math.random directly.
> > > 
> > > Done. PTAL.
> > 
> > lgtm.
> > 
> > however, this still requires MathRandom to not guard against zero-length
typed
> > array. I could imagine that, were it implemented slightly differently, it
> might
> > simply carry on.
> > 
> > We could install a getter for the length property that then either throws,
or
> > calls %Abort.
> 
> Fair enough. Updated.

assertDoesNotThrow would take a closure. The way it is used it doesnt do
anything, I think. It's also unnecessary. Let's remove it.

[gsathya, 2016-06-01 04:52:30]

On 2016/06/01 04:46:06, Yang wrote:
> On 2016/06/01 04:43:34, gsathya wrote:
> > On 2016/06/01 04:33:30, Yang wrote:
> > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > 
> > > > > > > can you reduce the test case a bit? The current one only fails
> because
> > > > > WeakMap
> > > > > > > happens to be based on Math.random internally.
> > > > > > 
> > > > > > I ended up removing the second test case (since they both fail
because
> > of
> > > > the
> > > > > > same thing). Let me know if you'd rather have a trimmed version of
the
> > > > second
> > > > > > test case like in patchset3
> > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > 
> > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > 
> > > > Done. PTAL.
> > > 
> > > lgtm.
> > > 
> > > however, this still requires MathRandom to not guard against zero-length
> typed
> > > array. I could imagine that, were it implemented slightly differently, it
> > might
> > > simply carry on.
> > > 
> > > We could install a getter for the length property that then either throws,
> or
> > > calls %Abort.
> > 
> > Fair enough. Updated.
> 
> assertDoesNotThrow would take a closure. The way it is used it doesnt do
> anything, I think. It's also unnecessary. Let's remove it.

assertDoesNotThrow evals code, so it should work, but agreed that it isn't
strictly necessary. Removed.

[gsathya, 2016-06-01 04:55:02]

On 2016/06/01 04:52:30, gsathya wrote:
> On 2016/06/01 04:46:06, Yang wrote:
> > On 2016/06/01 04:43:34, gsathya wrote:
> > > On 2016/06/01 04:33:30, Yang wrote:
> > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > 
> > > > > > > > can you reduce the test case a bit? The current one only fails
> > because
> > > > > > WeakMap
> > > > > > > > happens to be based on Math.random internally.
> > > > > > > 
> > > > > > > I ended up removing the second test case (since they both fail
> because
> > > of
> > > > > the
> > > > > > > same thing). Let me know if you'd rather have a trimmed version of
> the
> > > > > second
> > > > > > > test case like in patchset3
> > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > 
> > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > 
> > > > > Done. PTAL.
> > > > 
> > > > lgtm.
> > > > 
> > > > however, this still requires MathRandom to not guard against zero-length
> > typed
> > > > array. I could imagine that, were it implemented slightly differently,
it
> > > might
> > > > simply carry on.
> > > > 
> > > > We could install a getter for the length property that then either
throws,
> > or
> > > > calls %Abort.
> > > 
> > > Fair enough. Updated.
> > 
> > assertDoesNotThrow would take a closure. The way it is used it doesnt do
> > anything, I think. It's also unnecessary. Let's remove it.
> 
> assertDoesNotThrow evals code, so it should work, but agreed that it isn't
> strictly necessary. Removed.

Err, thought I had the calls within quotes. You're right it doesn't work.
Anyways, removed.

[Yang, 2016-06-01 05:01:48]

On 2016/06/01 04:55:02, gsathya wrote:
> On 2016/06/01 04:52:30, gsathya wrote:
> > On 2016/06/01 04:46:06, Yang wrote:
> > > On 2016/06/01 04:43:34, gsathya wrote:
> > > > On 2016/06/01 04:33:30, Yang wrote:
> > > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > > 
> > > > > > > > > can you reduce the test case a bit? The current one only fails
> > > because
> > > > > > > WeakMap
> > > > > > > > > happens to be based on Math.random internally.
> > > > > > > > 
> > > > > > > > I ended up removing the second test case (since they both fail
> > because
> > > > of
> > > > > > the
> > > > > > > > same thing). Let me know if you'd rather have a trimmed version
of
> > the
> > > > > > second
> > > > > > > > test case like in patchset3
> > > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > > 
> > > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > > 
> > > > > > Done. PTAL.
> > > > > 
> > > > > lgtm.
> > > > > 
> > > > > however, this still requires MathRandom to not guard against
zero-length
> > > typed
> > > > > array. I could imagine that, were it implemented slightly differently,
> it
> > > > might
> > > > > simply carry on.
> > > > > 
> > > > > We could install a getter for the length property that then either
> throws,
> > > or
> > > > > calls %Abort.
> > > > 
> > > > Fair enough. Updated.
> > > 
> > > assertDoesNotThrow would take a closure. The way it is used it doesnt do
> > > anything, I think. It's also unnecessary. Let's remove it.
> > 
> > assertDoesNotThrow evals code, so it should work, but agreed that it isn't
> > strictly necessary. Removed.
> 
> Err, thought I had the calls within quotes. You're right it doesn't work.
> Anyways, removed.

lgtm.

[gsathya, 2016-06-01 15:56:34]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-06-01 15:56:41]

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

[commit-bot: I haz the power, 2016-06-01 16:11:08]

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

[commit-bot: I haz the power, 2016-06-01 16:11:09]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[gsathya, 2016-06-01 17:31:54]

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

[commit-bot: I haz the power, 2016-06-01 17:32:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020203006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2020203006/200001

[commit-bot: I haz the power, 2016-06-01 17:59:21]

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

[commit-bot: I haz the power, 2016-06-01 17:59:22]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-06-01 18:00:31]

On 2016/06/01 05:01:48, Yang wrote:
> On 2016/06/01 04:55:02, gsathya wrote:
> > On 2016/06/01 04:52:30, gsathya wrote:
> > > On 2016/06/01 04:46:06, Yang wrote:
> > > > On 2016/06/01 04:43:34, gsathya wrote:
> > > > > On 2016/06/01 04:33:30, Yang wrote:
> > > > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > > > 
> > > > > > > > > > can you reduce the test case a bit? The current one only
fails
> > > > because
> > > > > > > > WeakMap
> > > > > > > > > > happens to be based on Math.random internally.
> > > > > > > > > 
> > > > > > > > > I ended up removing the second test case (since they both fail
> > > because
> > > > > of
> > > > > > > the
> > > > > > > > > same thing). Let me know if you'd rather have a trimmed
version
> of
> > > the
> > > > > > > second
> > > > > > > > > test case like in patchset3
> > > > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > > > 
> > > > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > > > 
> > > > > > > Done. PTAL.
> > > > > > 
> > > > > > lgtm.
> > > > > > 
> > > > > > however, this still requires MathRandom to not guard against
> zero-length
> > > > typed
> > > > > > array. I could imagine that, were it implemented slightly
differently,
> > it
> > > > > might
> > > > > > simply carry on.
> > > > > > 
> > > > > > We could install a getter for the length property that then either
> > throws,
> > > > or
> > > > > > calls %Abort.
> > > > > 
> > > > > Fair enough. Updated.
> > > > 
> > > > assertDoesNotThrow would take a closure. The way it is used it doesnt do
> > > > anything, I think. It's also unnecessary. Let's remove it.
> > > 
> > > assertDoesNotThrow evals code, so it should work, but agreed that it isn't
> > > strictly necessary. Removed.
> > 
> > Err, thought I had the calls within quotes. You're right it doesn't work.
> > Anyways, removed.
> 
> lgtm.

PTAL. Turns out GenerateRandonNumbers also returns a JSArray during snapshot
phase, so I need a IsTypedArray check.

[Yang, 2016-06-01 18:04:24]

On 2016/06/01 18:00:31, gsathya wrote:
> On 2016/06/01 05:01:48, Yang wrote:
> > On 2016/06/01 04:55:02, gsathya wrote:
> > > On 2016/06/01 04:52:30, gsathya wrote:
> > > > On 2016/06/01 04:46:06, Yang wrote:
> > > > > On 2016/06/01 04:43:34, gsathya wrote:
> > > > > > On 2016/06/01 04:33:30, Yang wrote:
> > > > > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > > > > 
> > > > > > > > > > > can you reduce the test case a bit? The current one only
> fails
> > > > > because
> > > > > > > > > WeakMap
> > > > > > > > > > > happens to be based on Math.random internally.
> > > > > > > > > > 
> > > > > > > > > > I ended up removing the second test case (since they both
fail
> > > > because
> > > > > > of
> > > > > > > > the
> > > > > > > > > > same thing). Let me know if you'd rather have a trimmed
> version
> > of
> > > > the
> > > > > > > > second
> > > > > > > > > > test case like in patchset3
> > > > > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > > > > 
> > > > > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > > > > 
> > > > > > > > Done. PTAL.
> > > > > > > 
> > > > > > > lgtm.
> > > > > > > 
> > > > > > > however, this still requires MathRandom to not guard against
> > zero-length
> > > > > typed
> > > > > > > array. I could imagine that, were it implemented slightly
> differently,
> > > it
> > > > > > might
> > > > > > > simply carry on.
> > > > > > > 
> > > > > > > We could install a getter for the length property that then either
> > > throws,
> > > > > or
> > > > > > > calls %Abort.
> > > > > > 
> > > > > > Fair enough. Updated.
> > > > > 
> > > > > assertDoesNotThrow would take a closure. The way it is used it doesnt
do
> > > > > anything, I think. It's also unnecessary. Let's remove it.
> > > > 
> > > > assertDoesNotThrow evals code, so it should work, but agreed that it
isn't
> > > > strictly necessary. Removed.
> > > 
> > > Err, thought I had the calls within quotes. You're right it doesn't work.
> > > Anyways, removed.
> > 
> > lgtm.
> 
> PTAL. Turns out GenerateRandonNumbers also returns a JSArray during snapshot
> phase, so I need a IsTypedArray check.

This only happens if we call Math.random before creating the snapshot. Which is
not the usual case, and only used for custom startup snapshot. Can you check
whether there is any performance regression for Math.random()? A micro-benchmark
should suffice. If there is a regression, let's disable that one test that tests
Math.random in custom startup snapshot for now, file a bug for that, and assign
it to me.

[gsathya, 2016-06-01 18:27:15]

On 2016/06/01 18:04:24, Yang wrote:
> On 2016/06/01 18:00:31, gsathya wrote:
> > On 2016/06/01 05:01:48, Yang wrote:
> > > On 2016/06/01 04:55:02, gsathya wrote:
> > > > On 2016/06/01 04:52:30, gsathya wrote:
> > > > > On 2016/06/01 04:46:06, Yang wrote:
> > > > > > On 2016/06/01 04:43:34, gsathya wrote:
> > > > > > > On 2016/06/01 04:33:30, Yang wrote:
> > > > > > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > can you reduce the test case a bit? The current one only
> > fails
> > > > > > because
> > > > > > > > > > WeakMap
> > > > > > > > > > > > happens to be based on Math.random internally.
> > > > > > > > > > > 
> > > > > > > > > > > I ended up removing the second test case (since they both
> fail
> > > > > because
> > > > > > > of
> > > > > > > > > the
> > > > > > > > > > > same thing). Let me know if you'd rather have a trimmed
> > version
> > > of
> > > > > the
> > > > > > > > > second
> > > > > > > > > > > test case like in patchset3
> > > > > > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > > > > > 
> > > > > > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > > > > > 
> > > > > > > > > Done. PTAL.
> > > > > > > > 
> > > > > > > > lgtm.
> > > > > > > > 
> > > > > > > > however, this still requires MathRandom to not guard against
> > > zero-length
> > > > > > typed
> > > > > > > > array. I could imagine that, were it implemented slightly
> > differently,
> > > > it
> > > > > > > might
> > > > > > > > simply carry on.
> > > > > > > > 
> > > > > > > > We could install a getter for the length property that then
either
> > > > throws,
> > > > > > or
> > > > > > > > calls %Abort.
> > > > > > > 
> > > > > > > Fair enough. Updated.
> > > > > > 
> > > > > > assertDoesNotThrow would take a closure. The way it is used it
doesnt
> do
> > > > > > anything, I think. It's also unnecessary. Let's remove it.
> > > > > 
> > > > > assertDoesNotThrow evals code, so it should work, but agreed that it
> isn't
> > > > > strictly necessary. Removed.
> > > > 
> > > > Err, thought I had the calls within quotes. You're right it doesn't
work.
> > > > Anyways, removed.
> > > 
> > > lgtm.
> > 
> > PTAL. Turns out GenerateRandonNumbers also returns a JSArray during snapshot
> > phase, so I need a IsTypedArray check.
> 
> This only happens if we call Math.random before creating the snapshot. Which
is
> not the usual case, and only used for custom startup snapshot. Can you check
> whether there is any performance regression for Math.random()? A
micro-benchmark
> should suffice. If there is a regression, let's disable that one test that
tests
> Math.random in custom startup snapshot for now, file a bug for that, and
assign
> it to me.

I don't see any regressions.

[Yang, 2016-06-01 18:31:26]

On 2016/06/01 18:27:15, gsathya wrote:
> On 2016/06/01 18:04:24, Yang wrote:
> > On 2016/06/01 18:00:31, gsathya wrote:
> > > On 2016/06/01 05:01:48, Yang wrote:
> > > > On 2016/06/01 04:55:02, gsathya wrote:
> > > > > On 2016/06/01 04:52:30, gsathya wrote:
> > > > > > On 2016/06/01 04:46:06, Yang wrote:
> > > > > > > On 2016/06/01 04:43:34, gsathya wrote:
> > > > > > > > On 2016/06/01 04:33:30, Yang wrote:
> > > > > > > > > On 2016/06/01 04:27:08, gsathya wrote:
> > > > > > > > > > On 2016/06/01 03:45:03, Yang wrote:
> > > > > > > > > > > On 2016/06/01 03:41:58, gsathya wrote:
> > > > > > > > > > > > On 2016/06/01 03:29:51, Yang wrote:
> > > > > > > > > > > > > On 2016/06/01 03:06:34, gsathya wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > can you reduce the test case a bit? The current one
only
> > > fails
> > > > > > > because
> > > > > > > > > > > WeakMap
> > > > > > > > > > > > > happens to be based on Math.random internally.
> > > > > > > > > > > > 
> > > > > > > > > > > > I ended up removing the second test case (since they
both
> > fail
> > > > > > because
> > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > > > same thing). Let me know if you'd rather have a trimmed
> > > version
> > > > of
> > > > > > the
> > > > > > > > > > second
> > > > > > > > > > > > test case like in patchset3
> > > > > > > > > > > > (https://codereview.chromium.org/2020203006/#ps40001)
> > > > > > > > > > > 
> > > > > > > > > > > lgtm. What I had in mind was to call Math.random directly.
> > > > > > > > > > 
> > > > > > > > > > Done. PTAL.
> > > > > > > > > 
> > > > > > > > > lgtm.
> > > > > > > > > 
> > > > > > > > > however, this still requires MathRandom to not guard against
> > > > zero-length
> > > > > > > typed
> > > > > > > > > array. I could imagine that, were it implemented slightly
> > > differently,
> > > > > it
> > > > > > > > might
> > > > > > > > > simply carry on.
> > > > > > > > > 
> > > > > > > > > We could install a getter for the length property that then
> either
> > > > > throws,
> > > > > > > or
> > > > > > > > > calls %Abort.
> > > > > > > > 
> > > > > > > > Fair enough. Updated.
> > > > > > > 
> > > > > > > assertDoesNotThrow would take a closure. The way it is used it
> doesnt
> > do
> > > > > > > anything, I think. It's also unnecessary. Let's remove it.
> > > > > > 
> > > > > > assertDoesNotThrow evals code, so it should work, but agreed that it
> > isn't
> > > > > > strictly necessary. Removed.
> > > > > 
> > > > > Err, thought I had the calls within quotes. You're right it doesn't
> work.
> > > > > Anyways, removed.
> > > > 
> > > > lgtm.
> > > 
> > > PTAL. Turns out GenerateRandonNumbers also returns a JSArray during
snapshot
> > > phase, so I need a IsTypedArray check.
> > 
> > This only happens if we call Math.random before creating the snapshot. Which
> is
> > not the usual case, and only used for custom startup snapshot. Can you check
> > whether there is any performance regression for Math.random()? A
> micro-benchmark
> > should suffice. If there is a regression, let's disable that one test that
> tests
> > Math.random in custom startup snapshot for now, file a bug for that, and
> assign
> > it to me.
> 
> I don't see any regressions.

LGTM. I remember that there are some Kraken or Sunspider benchmarks that rely on
Math.random, which is why I'm asking.

[gsathya, 2016-06-01 18:39:35]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-06-01 18:39:42]

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

[commit-bot: I haz the power, 2016-06-01 18:42:08]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776
==========

[commit-bot: I haz the power, 2016-06-01 18:42:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-01 18:44:43]

Description was changed from

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray 
length property writable, which means we shouldn't depend on it. 
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776
==========

to

==========
math.js: Use %_TypedArrayGetLength to get length

https://codereview.chromium.org/2001393004 makes TypedArray
length property writable, which means we shouldn't depend on it.
Instead, use %_TypedArrayGetLength% to get length.

Attached regression test.

BUG=chromium:615776

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

[commit-bot: I haz the power, 2016-06-01 18:44:44]

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