Use deadline in IdleNotification.

Use deadline in IdleNotification.

BUG=417668
LOG=n

[Hannes Payer (out of office), 2014-11-25 08:23:21]

hpayer@chromium.org changed reviewers:
+ jochen@chromium.org, rmcilroy@chromium.org

[rmcilroy, 2014-11-25 10:35:41]

https://codereview.chromium.org/750813003/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/120001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4429: deadline -
V8::GetCurrentPlatform()->MonotonicallyIncreasingTime();
Both deadline and MonotonicallyIncreasingTime() return time in seconds, not ms. 
Probably better to rename the arguments to deadline_seconds to avoid this
confusion in the future.  Actually, it might be even better to translate to
base::TimeTicks / base::TimeDelta initially to avoid issues (I would have
returned base::TimeTicks for MonotonicallyIncreasingTime() if it was available
in Blink, but unfortunately it's not).

[Hannes Payer (out of office), 2014-11-25 12:28:59]

https://codereview.chromium.org/750813003/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/120001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4429: deadline -
V8::GetCurrentPlatform()->MonotonicallyIncreasingTime();
On 2014/11/25 10:35:41, rmcilroy wrote:
> Both deadline and MonotonicallyIncreasingTime() return time in seconds, not
ms. 
> Probably better to rename the arguments to deadline_seconds to avoid this
> confusion in the future.  Actually, it might be even better to translate to
> base::TimeTicks / base::TimeDelta initially to avoid issues (I would have
> returned base::TimeTicks for MonotonicallyIncreasingTime() if it was available
> in Blink, but unfortunately it's not).

I am using now seconds granularity on the api level and internally I convert it
to milliseconds.

[rmcilroy, 2014-11-25 14:28:06]

https://codereview.chromium.org/750813003/diff/130001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/750813003/diff/130001/include/v8.h#newcode5001
include/v8.h:5001: * done within the time limit.
nit - mention that deadline_seconds is compared with
MonotonicallyIncreasingTime() and should be based on the same timebase as that
function.

https://codereview.chromium.org/750813003/diff/130001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/api.cc#newcode6637
src/api.cc:6637: return isolate->heap()->IdleNotification(deadline_in_seconds);
you need to multiply by 1000 right?  Could we just convert to base::TimeTicks
internally to avoid the confusion?

https://codereview.chromium.org/750813003/diff/130001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:169: GCIdleTimeAction
GCIdleTimeHandler::Compute(double idle_time,
nit - keep this as idle_time_in_ms

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4398: static_cast<double>(idle_time_in_ms));
indentation.

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4430: deadline_in_ms -
V8::GetCurrentPlatform()->MonotonicallyIncreasingTime();
still subtracting seconds from ms here.

[jochen (gone - plz use gerrit), 2014-11-25 15:54:52]

i'm worried that somewhere in chrome we actually pass in a float value that
currently gets automatically converted to an int but then would be interpreted
as a deadline?

[Hannes Payer (out of office), 2014-11-25 15:57:24]

On 2014/11/25 15:54:52, jochen (slow) wrote:
> i'm worried that somewhere in chrome we actually pass in a float value that
> currently gets automatically converted to an int but then would be interpreted
> as a deadline?

I decided to stick to raw values. TimeTicks would be nice but there are still
cases that require extra casting, e.g. to size_t when we do calculations
including memory limits.

[Hannes Payer (out of office), 2014-11-25 15:58:07]

On 2014/11/25 15:57:24, Hannes Payer wrote:
> On 2014/11/25 15:54:52, jochen (slow) wrote:
> > i'm worried that somewhere in chrome we actually pass in a float value that
> > currently gets automatically converted to an int but then would be
interpreted
> > as a deadline?
> 
> I decided to stick to raw values. TimeTicks would be nice but there are still
> cases that require extra casting, e.g. to size_t when we do calculations
> including memory limits.

We can make it explicit in the api. IdleNotificationDeadline?

[Hannes Payer (out of office), 2014-11-25 16:01:37]

https://codereview.chromium.org/750813003/diff/130001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/750813003/diff/130001/include/v8.h#newcode5001
include/v8.h:5001: * done within the time limit.
On 2014/11/25 14:28:06, rmcilroy wrote:
> nit - mention that deadline_seconds is compared with
> MonotonicallyIncreasingTime() and should be based on the same timebase as that
> function.

Done.

https://codereview.chromium.org/750813003/diff/130001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/api.cc#newcode6637
src/api.cc:6637: return isolate->heap()->IdleNotification(deadline_in_seconds);
On 2014/11/25 14:28:06, rmcilroy wrote:
> you need to multiply by 1000 right?  Could we just convert to base::TimeTicks
> internally to avoid the confusion?

I moved the conversion to the actual IdleNotification implementation.

https://codereview.chromium.org/750813003/diff/130001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:169: GCIdleTimeAction
GCIdleTimeHandler::Compute(double idle_time,
On 2014/11/25 14:28:06, rmcilroy wrote:
> nit - keep this as idle_time_in_ms

Done.

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4398: static_cast<double>(idle_time_in_ms));
On 2014/11/25 14:28:06, rmcilroy wrote:
> indentation.

gc format is formatting it like that.

https://codereview.chromium.org/750813003/diff/130001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4430: deadline_in_ms -
V8::GetCurrentPlatform()->MonotonicallyIncreasingTime();
On 2014/11/25 14:28:06, rmcilroy wrote:
> still subtracting seconds from ms here.

Done.

[rmcilroy, 2014-11-26 11:35:41]

Lgtm with a couple of final comments.

Also - you could link this with crbug.com/417668, which I've just made you owner
of :).

https://codereview.chromium.org/750813003/diff/310001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/750813003/diff/310001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:192: GCIdleTimeAction
GCIdleTimeHandler::Compute(double idle_time_in_ms,
Maybe just keep this as a size_t since you cast it to size_t whenever you use it
here.

https://codereview.chromium.org/750813003/diff/310001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/310001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4460: static_cast<double>(base::Time::kMillisecondsPerSecond));
nit - would be good to have a helper for MonotonicallyIncreasingTimeInMs() (in
Platform?) instead of doing this each time.

[Hannes Payer (out of office), 2014-11-27 12:19:15]

https://codereview.chromium.org/750813003/diff/310001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/750813003/diff/310001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:192: GCIdleTimeAction
GCIdleTimeHandler::Compute(double idle_time_in_ms,
On 2014/11/26 11:35:41, rmcilroy wrote:
> Maybe just keep this as a size_t since you cast it to size_t whenever you use
it
> here.

Nope, idle_time_in_ms can be negative in the deadline based approach. I think I
will do a cleanup cl that makes the time representation uniform.

https://codereview.chromium.org/750813003/diff/310001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/750813003/diff/310001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4460: static_cast<double>(base::Time::kMillisecondsPerSecond));
On 2014/11/26 11:35:41, rmcilroy wrote:
> nit - would be good to have a helper for MonotonicallyIncreasingTimeInMs() (in
> Platform?) instead of doing this each time.

Done.

[jochen (gone - plz use gerrit), 2014-11-27 13:02:16]

lgtm.

[Hannes Payer (out of office), 2014-11-27 13:06:58]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2014-11-27 13:07:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750813003/340001

[commit-bot: I haz the power, 2014-11-27 13:14:25]

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

[commit-bot: I haz the power, 2014-11-27 13:14:26]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Hannes Payer (out of office), 2014-11-28 08:10:19]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2014-11-28 08:10:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750813003/360001

[commit-bot: I haz the power, 2014-11-28 08:17:30]

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

[commit-bot: I haz the power, 2014-11-28 08:17:31]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Hannes Payer (out of office), 2014-11-28 09:33:51]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2014-11-28 09:34:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750813003/380001

[commit-bot: I haz the power, 2014-11-28 09:44:41]

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

[commit-bot: I haz the power, 2014-11-28 09:44:42]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Hannes Payer (out of office), 2014-11-28 10:25:32]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2014-11-28 10:26:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750813003/400001

[commit-bot: I haz the power, 2014-11-28 10:59:24]

Committed patchset #22 (id:400001)