Fixing --verify-predictable mode.

Fixing --verify-predictable mode.

This CL fixes several sources of non-predictability by making Platform::MonotonicallyIncreasingTime() the only bottleneck for all time-querying functions and providing PredictablePlatform implementation.

Committed: https://crrev.com/722e19efd646b98658bb9f6bf7af99d2eb10c537
Cr-Commit-Position: refs/heads/master@{#31959}

[Igor Sheludko, 2015-11-10 10:18:54]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-11-10 10:23:28]

Description was changed from

==========
WIP: Fixing --verify-predictable mode.
==========

to

==========
WIP: Fixing --verify-predictable mode.

This CL fixes several sources of non-predictability mostly by making
Platform::MonotonicallyIncreasingTime() the only bottleneck for all
time-querying functions.
==========

[Igor Sheludko, 2015-11-10 10:41:05]

ishell@chromium.org changed reviewers:
+ hpayer@chromium.org

[Igor Sheludko, 2015-11-10 10:41:06]

This is WIP but I would very appreciate your thought about this CL.

The changes in default-platform are a little bit controversial and there are
still issues with Octane which is still not predictable.

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
src/libplatform/default-platform.cc:14: #include "src/flags.h"
Unfortunately this violates "-src" directive from src/libplatform's
include_rules.

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
src/libplatform/default-platform.cc:169: if
(v8::internal::FLAG_verify_predictable) {
We could probably make FLAG_verify_predictable read-only and set it to true when
VERIFY_PREDICTABLE macro is enabled.
Thus we could write #ifdef VERIFY_PREDICTABLE here...

[jochen (gone - plz use gerrit), 2015-11-10 23:20:06]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-11-10 23:20:06]

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
src/libplatform/default-platform.cc:169: if
(v8::internal::FLAG_verify_predictable) {
On 2015/11/10 at 10:41:06, Igor Sheludko wrote:
> We could probably make FLAG_verify_predictable read-only and set it to true
when VERIFY_PREDICTABLE macro is enabled.
> Thus we could write #ifdef VERIFY_PREDICTABLE here...

we can't rely on every embedder doing this. how about just not using this
function in verify predictable mode?

[Igor Sheludko, 2015-11-11 10:20:46]

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/1415383004/diff/20001/src/libplatform/default...
src/libplatform/default-platform.cc:169: if
(v8::internal::FLAG_verify_predictable) {
On 2015/11/10 23:20:06, jochen (slow - traveling) wrote:
> On 2015/11/10 at 10:41:06, Igor Sheludko wrote:
> > We could probably make FLAG_verify_predictable read-only and set it to true
> when VERIFY_PREDICTABLE macro is enabled.
> > Thus we could write #ifdef VERIFY_PREDICTABLE here...
> 
> we can't rely on every embedder doing this. how about just not using this
> function in verify predictable mode?

1) The VERIFY_PREDICTABLE (and then --verify-predictable) is a special build&run
mode that was intended to help us to ensure that we know all sources of
non-determinism in V8 by calculating a hash sum of all allocations. I don't
think the embedders should care about this mode at all. Don't mix it up with
--predictable, which just disables threads and sets random seed.
2) Your idea is fine modulo the fact that several DefaultPlatform's methods also
call MonotonicallyIncreasingTime(), for example
DefaultPlatform::CallDelayedOnForegroundThread().

[Igor Sheludko, 2015-11-11 19:56:07]

Patchset #2 (id:40001) has been deleted

[Igor Sheludko, 2015-11-11 19:57:19]

Patchset #2 (id:60001) has been deleted

[Igor Sheludko, 2015-11-11 20:00:29]

PTAL

I added CreatePredictablePlatform() to libplatform.h instead. This approach
looks better to me. We could even remove declaration from the public header file
and declare the function somewhere in d8.cc. It's internal anyway...

[jochen (gone - plz use gerrit), 2015-11-11 20:38:08]

if this is just a d8 thing, maybe d8 should have its own v8::Platform
implementation that doesn't do threads at all (just run all tasks immediately
when they're posted), and returns this dummy time?

[Igor Sheludko, 2015-11-12 10:15:54]

Patchset #3 (id:100001) has been deleted

[Igor Sheludko, 2015-11-12 10:23:29]

Description was changed from

==========
WIP: Fixing --verify-predictable mode.

This CL fixes several sources of non-predictability mostly by making
Platform::MonotonicallyIncreasingTime() the only bottleneck for all
time-querying functions.
==========

to

==========
Fixing --verify-predictable mode.

This CL fixes several sources of non-predictability by making
Platform::MonotonicallyIncreasingTime() the only bottleneck for all
time-querying functions and providing PredictablePlatform implementation.
==========

[Igor Sheludko, 2015-11-12 10:43:17]

ishell@chromium.org changed reviewers:
+ ulan@chromium.org

[Igor Sheludko, 2015-11-12 10:43:18]

Ok, I added PredictablePlatform into d8.cc. Thanks, Jochen!

Now the CL is ready for review. Ulan, PTAL.

[ulan, 2015-11-12 11:38:15]

https://codereview.chromium.org/1415383004/diff/120001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1415383004/diff/120001/src/d8.cc#newcode126
src/d8.cc:126: double delay_in_seconds) override;
Maybe disable memory-reducer in predictable mode? (--nomemory-reducer) and just
provide a simple implementation that calls the task immediately?

https://codereview.chromium.org/1415383004/diff/120001/src/heap/heap-inl.h
File src/heap/heap-inl.h (right):

https://codereview.chromium.org/1415383004/diff/120001/src/heap/heap-inl.h#ne...
src/heap/heap-inl.h:261: ++allocations_count_;
Why the increment is conditional now?

[Igor Sheludko, 2015-11-12 11:53:30]

https://codereview.chromium.org/1415383004/diff/120001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1415383004/diff/120001/src/d8.cc#newcode126
src/d8.cc:126: double delay_in_seconds) override;
On 2015/11/12 11:38:15, ulan wrote:
> Maybe disable memory-reducer in predictable mode? (--nomemory-reducer) and
just
> provide a simple implementation that calls the task immediately?

Yes, it makes sense to disable the memory reducer in predictable mode, but the
--verify-predictable (when this platform is enabled) and --predictable are
currently two different flags. You can have the former one, but not the latter.

https://codereview.chromium.org/1415383004/diff/120001/src/heap/heap-inl.h
File src/heap/heap-inl.h (right):

https://codereview.chromium.org/1415383004/diff/120001/src/heap/heap-inl.h#ne...
src/heap/heap-inl.h:261: ++allocations_count_;
On 2015/11/12 11:38:15, ulan wrote:
> Why the increment is conditional now?

We use allocations_count only for --verify-predictable and/or
--trace-allocation-stack-interval. I just moved the increment below the flags
(here and below).

[Igor Sheludko, 2015-11-12 11:54:33]

Patchset #4 (id:140001) has been deleted

[ulan, 2015-11-12 12:27:58]

lgtm

[Igor Sheludko, 2015-11-12 12:42:33]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-11-12 12:42:44]

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

[commit-bot: I haz the power, 2015-11-12 13:02:31]

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

[commit-bot: I haz the power, 2015-11-12 13:02:32]

Try jobs failed on following builders:
  v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)

[Igor Sheludko, 2015-11-12 13:25:05]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2015-11-12 13:25:06]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/1415383004/#ps220001
(title: "Fixed shared build")

[commit-bot: I haz the power, 2015-11-12 13:25:13]

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

[commit-bot: I haz the power, 2015-11-12 13:42:56]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-11-14 23:20:39]

Patchset 7 (id:??) landed as
https://crrev.com/722e19efd646b98658bb9f6bf7af99d2eb10c537
Cr-Commit-Position: refs/heads/master@{#31959}

[Lei Zhang, 2015-11-20 06:44:13]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-11-20 06:44:14]

Hi, I couldn't roll DEPS for V8 in PDFium due to a Windows build warning. This
week, my attempt failed with this CL. To repro on Linux:

1) mkdir foo
2) cd foo
3) fetch pdfium
4) cd pdfium/v8
5) sync to this revision
6) cd ..
7) gclient runhooks
8) ninja -C out/Debug pdfium_embeddertests
9) out/Debug/pdfium_embeddertests --gtest_filter=FXJSV8Embeddertest.Getters

This crashes with:

# Fatal error in ../../v8/src/isolate.cc, line 1803
# Check failed: thread_data_table_.

Do we need to take some action in pdfium or is this a V8 bug?

[Igor Sheludko, 2015-11-20 08:11:49]

On 2015/11/20 06:44:14, Lei Zhang wrote:
> Hi, I couldn't roll DEPS for V8 in PDFium due to a Windows build warning. This
> week, my attempt failed with this CL. To repro on Linux:
> 
> 1) mkdir foo
> 2) cd foo
> 3) fetch pdfium
> 4) cd pdfium/v8
> 5) sync to this revision
> 6) cd ..
> 7) gclient runhooks
> 8) ninja -C out/Debug pdfium_embeddertests
> 9) out/Debug/pdfium_embeddertests --gtest_filter=FXJSV8Embeddertest.Getters
> 
> This crashes with:
> 
> # Fatal error in ../../v8/src/isolate.cc, line 1803
> # Check failed: thread_data_table_.
> 
> Do we need to take some action in pdfium or is this a V8 bug?

I don't see how this problem is related to my CL.

What kind of build warning do you see?

Regarding the thread_data_table_ failure, I would guess that the
Isolate::InitializeOncePerProcess() is not called by the test.

[jochen (gone - plz use gerrit), 2015-11-20 08:29:34]

Lei, thanks for bisecting this. If you run into problems like this in the
future, feel free to just file a bug and assign it to me

[Lei Zhang, 2015-11-20 21:09:09]

On 2015/11/20 08:29:34, jochen wrote:
> Lei, thanks for bisecting this. If you run into problems like this in the
> future, feel free to just file a bug and assign it to me

Thanks. I started with the assumption that the test was correct, but that was
not the case.