Project: v8 Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 4 users
Status: Fixed
Owner: ----
Closed: Mar 2016
Cc:
Components:
HW: ----
OS: ----
Priority: 2
Type: Bug



Sign in to add a comment
Math.random() does not work with Snapshots
Reported by m...@techpivot.net, Mar 5 2016 Back to list
Version: 5.0.x, 5.1.x
OS: Ubuntu 14.04
Architecture: x64

What steps will reproduce the problem?
1.Create test.js
```
function getRandom() {
  return Math.random();
}

var randomValue = Math.random();
var randomValueFromFunction = getRandom();
```

2. Create a snapshot
```
user@system:~/git/v8/out/native$ ./mksnapshot --startup_blob=snapshot_blob.bin test.js                                                      
Embedding extra script: test.js
```

3. user@system:~/git/v8/out/native$ ./shell 
V8 version 5.1.53 [sample shell]
> print(randomValue);
0.47515791309267663
> print(randomValueFromFunction);
0.05808531226718627
> print(getRandom());
<embedded script>:6: TypeError: Cannot read property '4' of undefined
  return Math.random();
              ^
TypeError: Cannot read property '4' of undefined
    at Object.random (native)
    at getRandom (<embedded script>:6:15)
    at (shell):1:7


What is the expected output?
Math.random() should be able to generate a random value at runtime.


For additional information, see https://github.com/phpv8/v8js/pull/207

 
Components: Runtime
Owner: yangguo@chromium.org
Status: Assigned
Well, it should not be able to generate different random values but it should not crash. See http://v8project.blogspot.de/2015/09/custom-startup-snapshots.html for more information on this.
If you ran the repro in debug mode build, mksnapshot would have already triggered a check failure. Math.random is prohibited during snapshot creation for a good reason. If you only create a function that references Math.random, that's fine. But if you actually call into Math.random, then the result, if it is put into the snapshot, would not be random anymore.

Since we don't support called Math.random when creating the snapshot, we assume that this does not happen. If it however does get called, somethings break.

I'm adding a release build check that Math.random does not get called when creating the snapshot.
Status: WorkingAsIntended
Project Member Comment 4 by bugdroid1@chromium.org, Mar 8 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/232cd81aba334639be0ac1047ee03345ece805d3

commit 232cd81aba334639be0ac1047ee03345ece805d3
Author: yangguo <yangguo@chromium.org>
Date: Tue Mar 08 12:45:31 2016

Math.random must not be executed when creating a snapshot.

Previously, the assertion does not include code executed in
the custom heap snapshot.

TBR=hablich@chromium.org
BUG= v8:4810 
LOG=N

Review URL: https://codereview.chromium.org/1771313002

Cr-Commit-Position: refs/heads/master@{#34583}

[modify] https://crrev.com/232cd81aba334639be0ac1047ee03345ece805d3/src/runtime/runtime-maths.cc

Comment 5 by m...@techpivot.net, Mar 8 2016
Can you clarify with regards to:

"If you only create a function that references Math.random, that's fine. But if you actually call into Math.random, then the result, if it is put into the snapshot, would not be random anymore."

According to the example listed above for function getRandom(), it's not called during snapshot creation. Instead it's called at runtime. Shouldn't that work according to above? 
Don't you call Math.random twice to initialize 'randomValue' and 'randomValueFromFunction ' when you create the snapshot? You are not supposed to do that, and on a debug build, that would trigger check failures.
Comment 7 by m...@techpivot.net, Mar 8 2016
Ignore the variables. What about at runtime? 

```
function getRandom() {
  return Math.random();
}
```

* Take snapshot, then

$ ./shell 
V8 version 5.1.53 [sample shell]
> print(getRandom());
> print(Math.random());


Shouldn't this work?
Yes. Doesn't it?
I just verified. If you don't call getRandom() while creating the snapshot, everything works just fine.
I hadn't had a chance to test, thank you. 

To confirm/clarify for my understanding ... Taking a snapshot where the Math.random() is executed (at least once) within the snapshot code baseline will evaluate all the snapshot-time [ Math.random() ] instances correctly; however, subsequent calls into Math.random() via V8 with snapshot loaded will fail. When no calls into [ Math.random() ] are made at snapshot creation, subsequent V8 calls into Math.random() work fine.

If this is correct ... doesn't this imply there is a bug? Shouldn't any call into Math.random() at runtime after the snapshot generation work regardless of what the snapshot did? I just did read the article referenced which seems to confirm this. 

"And of course, values derived from sources such as Math.random or Date.now are fixed once the snapshot has been captured."

I guess I'm looking for slightly more information. Intuitively it doesn't make sense that if the function renders once then V8 runtime functions will no longer work. Wouldn't it be an easy fix to just re-initialize the Math.random()?

Maybe I missed 
something. Thank you for your clarification.
The scenario you are describing is not supported. Math.random is not supposed to be called at snapshot time at all. If you ran your test in debug build, it would fail, intentionally. Values generated by Math.random is not random, and I'd like to prevent anybody from making the mistake of calling it at snapshot time.

Unrelated to this imposed limitation, there is also a technical limitation: Math.random has an internal state that cannot be fully serialized into the snapshot. The solution to this technical limitation would be to reinitialize the internal state after deserialization, but given the imposed limitation aboce, there is no point in doing that.

I have since changed the check to fail for release builds as well. Just don't call Math.random when creating the snapshot. Use a hard-coded "random" value if you really need one.
@yanggou, Thanks for the follow up.

One last follow up in an attempt to not have to deal with frameworks that utilize Math.random() 

Would it be possible to just exclude the Math.random() from serialization all together? That way no reinitialization of the internal state would need to take place (It would also be more cost effective as snapshot frequently is low, yet initialization is high). 

That way, snapshots that use it, fine. And runtimes that require it use the state as normally initialized via V8. 
The serializer is pretty generic and I'd like to keep it that way.

If your problem is that you are running existing code when creating the snapshot and have no way to avoid calling to Math.random, why don't you overwrite Math.random? Something like

var __math_random__ = Math.random;
Math.random = function() { return 0.5; }

[ insert your framework code here ]

Math.random = __math_random__;

What I definitely want to prevent is for users to accidentally call Math.random() when creating the snapshot, and end up with a value that's then no longer random. The way to do this is crash with a check failure. What you are suggesting is to carry on silently, which is a bad idea imo.
@yanggou  .. We actually override the math.random call (See bottom of  https://github.com/phpv8/v8js/pull/207) as we need this working in production immediately (So yes, that is what we do).

I was hoping that a serialization update would be trivial and that way end users wouldn't have to worry about anything :)
Like I explained previously, not working is by design, to prevent accidental calls to Math.random. I don't see a good way to warn users of Math.random not being random other than a check failure. The end user *should* worry about this.
Status: Assigned
Now that I think about this, Date.now should be treated equally.

Let me think about how we should approach this. Maybe it is OK to have calls to Math.random, at the risk of silently embedding non-random values into the snapshot.
Comment 18 by m...@techpivot.net, Mar 10 2016
@yangguo

I agree with your last comment. I do think that embedding the non-random values is fine (and honestly the expected behavior). My biggest concern is just ensuring that post-snapshot runtime works as intended. That is why my suggestion of not serializing the heap associated with Math.random/Date.now seems like the most logical approach such that initialization of the V8 engine would not have to do anything differently for the startup for these calls.
oups. tagged the bug number wrong.
Status: Fixed
Cc: yangguo@chromium.org
Owner: ----
Status: Available
The fix regresses performance. I'm reopening this, but don't plan to work on this in the near future though.
Project Member Comment 24 by bugdroid1@chromium.org, Mar 14 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b7be51cd33bc81d768dbf5632ba0c68843448e37

commit b7be51cd33bc81d768dbf5632ba0c68843448e37
Author: yangguo <yangguo@chromium.org>
Date: Mon Mar 14 10:26:17 2016

Revert of Allow Math.random to be called when creating a custom startup snapshot. (patchset #2 id:20001 of https://codereview.chromium.org/1780173002/ )

Reason for revert:
Regresses performance on base64 benchmark.

Original issue's description:
> Allow Math.random to be called when creating a custom startup snapshot.
>
> R=jkummerow@chromium.org
> BUG= v8:4810 
> LOG=N
>
> Committed: https://crrev.com/6a7ec6a3bf779cdd41c66a768fd7a37195ed7b7f
> Cr-Commit-Position: refs/heads/master@{#34705}

TBR=jkummerow@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= v8:4810 , chromium:594484
LOG=N

Review URL: https://codereview.chromium.org/1798863003

Cr-Commit-Position: refs/heads/master@{#34739}

[modify] https://crrev.com/b7be51cd33bc81d768dbf5632ba0c68843448e37/src/js/math.js
[modify] https://crrev.com/b7be51cd33bc81d768dbf5632ba0c68843448e37/src/runtime/runtime-maths.cc
[modify] https://crrev.com/b7be51cd33bc81d768dbf5632ba0c68843448e37/test/cctest/test-serialize.cc

Comment 25 by m...@techpivot.net, Mar 14 2016
@yangguo 

Just to confirm:

* Compilation still works when using snapshots with Date.now(), Math.random()
* Current behavior as outlined in bug report still exists. E.g. Future calls to Math.random() will error if Math.random() is used at least once during snapshot generation.

Is the current workaround when using Math.random() inside snapshots to re-initialize/override math.random() as necessary in subsequent context?

- Referring to Math.random() in code included in the snapshot will still work.
- Calling Math.random() when creating the snapshot will cause a check failure (crash).
- Workaround is to override Math.random for the snapshot.
Project Member Comment 27 by bugdroid1@chromium.org, Mar 16 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/4513e07787e2860a73d290d7580c961a0aad3482

commit 4513e07787e2860a73d290d7580c961a0aad3482
Author: yangguo <yangguo@chromium.org>
Date: Wed Mar 16 15:02:41 2016

Reland of Allow Math.random to be called when creating a custom startup snapshot. (patchset #1 id:1 of https://codereview.chromium.org/1798863003/ )

Reason for revert:
This seems not to change performance.

Original issue's description:
> Revert of Allow Math.random to be called when creating a custom startup snapshot. (patchset #2 id:20001 of https://codereview.chromium.org/1780173002/ )
>
> Reason for revert:
> Regresses performance on base64 benchmark.
>
> Original issue's description:
> > Allow Math.random to be called when creating a custom startup snapshot.
> >
> > R=jkummerow@chromium.org
> > BUG= v8:4810 
> > LOG=N
> >
> > Committed: https://crrev.com/6a7ec6a3bf779cdd41c66a768fd7a37195ed7b7f
> > Cr-Commit-Position: refs/heads/master@{#34705}
>
> TBR=jkummerow@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= v8:4810 , chromium:594484
> LOG=N
>
> Committed: https://crrev.com/b7be51cd33bc81d768dbf5632ba0c68843448e37
> Cr-Commit-Position: refs/heads/master@{#34739}

TBR=jkummerow@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= v8:4810 , chromium:594484
LOG=N

Review URL: https://codereview.chromium.org/1806713003

Cr-Commit-Position: refs/heads/master@{#34820}

[modify] https://crrev.com/4513e07787e2860a73d290d7580c961a0aad3482/src/js/math.js
[modify] https://crrev.com/4513e07787e2860a73d290d7580c961a0aad3482/src/runtime/runtime-maths.cc
[modify] https://crrev.com/4513e07787e2860a73d290d7580c961a0aad3482/test/cctest/test-serialize.cc

Status: Fixed
Comment 30 by m...@techpivot.net, Mar 31 2016
Thank you !
Labels: Priority-2
Sign in to add a comment