chrome://profiler infrastructure uses base time types.

chrome://profiler infrastructure uses base time types.

Previously, chrome://profiler avoided base::TimeTicks and
base::TimeDelta because they're 64 bits.

At this point, that appears to be premature optimization. Switch to
using the base time types.

This is required for using chrome://profiler's infrastructure for
building thread time UMA metrics.

BUG=736449
Review-Url: https://codereview.chromium.org/2956683002
Cr-Commit-Position: refs/heads/master@{#483054}
Committed: https://chromium.googlesource.com/chromium/src/+/6f2691cc7756d599ce5622cb7c9fc854afcf3c57

[tdresser, 2017-06-23 18:57:28]

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

[commit-bot: I haz the power, 2017-06-23 18:57:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 19:12:39]

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

[commit-bot: I haz the power, 2017-06-23 19:12:40]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[tdresser, 2017-06-26 14:10:03]

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

[commit-bot: I haz the power, 2017-06-26 14:10:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdresser, 2017-06-26 15:36:10]

tdresser@chromium.org changed reviewers:
+ gab@chromium.org

[tdresser, 2017-06-26 15:36:11]

I'm hoping to build a CPU time UMA on top of the profiler architecture, and it
would be nice to be able to use standard time types.

There may be some fundamental reason this is a bad idea, but if so, I'm not sure
what it is. :)

The current justification from the comments is that using a 64 bit data type is
"wasteful".

[commit-bot: I haz the power, 2017-06-26 17:01:30]

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

[commit-bot: I haz the power, 2017-06-26 17:01:31]

Dry run: This issue passed the CQ dry run.

[gab, 2017-06-26 21:48:12]

I agree that we've likely crossed the "wasteful" barrier now that we ship 64-bit
builds by default (and all pointers are thus also "wasteful").

I also like simplifying base types :).

A few things below:

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:541: random_number_ ^= (Now() -
base::TimeTicks()).InMilliseconds();
This is a bitwise XOR, not sure what it means to mix bitness here,
static_cast<uint32_t> the RHS to be safe?

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:938: 1000);
Why is this changing?

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:1047: excluded_duration_(),
Remove default members from initialization list.

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:349: const base::TimeDelta run_duration,
const non-ref method params is weird, but I guess you're not changing that..

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:692: const base::TimeDelta queue_duration,
const& ?

Either way, passing by value is preferred:
https://cs.chromium.org/chromium/src/base/time/time.h?sq=package:chromium&dr=...

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:657: const base::TimeTicks kTimePosted =
base::TimeTicks::FromInternalValue(1000);
Why FromIntervalValue() here and elsewhere in this file?

[gab, 2017-06-26 22:06:53]

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
File base/profiler/tracked_time.h (left):

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
base/profiler/tracked_time.h:18: // windows, a 64 bit timer is expensive to even
obtain. We use a simple
Oh also, what's this "64-bit timer" referring to? Is this number used outside of
base::Timer?

[gab, 2017-06-26 22:10:40]

Description was changed from

==========
chrome://profiler infrastructure uses base time types.

Previously, chrome://profiler avoided base::TimeTicks and
base::TimeDelta because they're 64 bits.

At this point, that appears to be premature optimization. Switch to
using the base time types.

This is required for using chrome://profiler's infrastructure for
building thread time UMA metrics.

BUG=736449
==========

to

==========
chrome://profiler infrastructure uses base time types.

Previously, chrome://profiler avoided base::TimeTicks and
base::TimeDelta because they're 64 bits.

At this point, that appears to be premature optimization. Switch to
using the base time types.

This is required for using chrome://profiler's infrastructure for
building thread time UMA metrics.

BUG=736449
==========

[gab, 2017-06-26 22:10:40]

gab@chromium.org changed reviewers:
+ stanisc@chromium.org

[gab, 2017-06-26 22:11:07]

On 2017/06/26 22:06:53, gab wrote:
>
https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
> File base/profiler/tracked_time.h (left):
> 
>
https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
> base/profiler/tracked_time.h:18: // windows, a 64 bit timer is expensive to
even
> obtain. We use a simple
> Oh also, what's this "64-bit timer" referring to? Is this number used outside
of
> base::Timer?

+stanisc for "64-bit timer on Windows" (per ownership @
https://bugs.chromium.org/p/chromium/issues/detail?id=736489)

[stanisc, 2017-06-26 22:33:21]

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
File base/profiler/tracked_time.h (left):

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
base/profiler/tracked_time.h:18: // windows, a 64 bit timer is expensive to even
obtain. We use a simple
On 2017/06/26 22:06:53, gab wrote:
> Oh also, what's this "64-bit timer" referring to? Is this number used outside
of
> base::Timer?

This probably refers to microsecond-precision TimeTicks value obtained via
QueryPerformanceCounter as opposed to a millisecond precision 32-bit value
returned by timeGetTime which is used as a backup when QPC method isn't well
supported by a plaform. See TimeTicks::IsHighResolution().

So the comment above is misleading when it mentions 64 bit timer.
There are two concepts that sound very similar but completely unrelated:

1) High resolution TimeTicks is what I've described above.
2) There is also high resolution system timer which affects only wait operations
with a timeout like Sleep or WaitForSingleObject and which is used for delayed
tasks. The default system timer frequency is 64 Hz which provides wakeup
precision to only about 15-16 ms. Activating the high resolution system timer
frequency gives us 1 ms wakeup precision.

[tdresser, 2017-06-27 13:21:56]

PTAL

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
File base/profiler/tracked_time.h (left):

https://codereview.chromium.org/2956683002/diff/20001/base/profiler/tracked_t...
base/profiler/tracked_time.h:18: // windows, a 64 bit timer is expensive to even
obtain. We use a simple
On 2017/06/26 22:33:21, stanisc wrote:
> On 2017/06/26 22:06:53, gab wrote:
> > Oh also, what's this "64-bit timer" referring to? Is this number used
outside
> of
> > base::Timer?
> 
> This probably refers to microsecond-precision TimeTicks value obtained via
> QueryPerformanceCounter as opposed to a millisecond precision 32-bit value
> returned by timeGetTime which is used as a backup when QPC method isn't well
> supported by a plaform. See TimeTicks::IsHighResolution().
> 
> So the comment above is misleading when it mentions 64 bit timer.
> There are two concepts that sound very similar but completely unrelated:
> 
> 1) High resolution TimeTicks is what I've described above.
> 2) There is also high resolution system timer which affects only wait
operations
> with a timeout like Sleep or WaitForSingleObject and which is used for delayed
> tasks. The default system timer frequency is 64 Hz which provides wakeup
> precision to only about 15-16 ms. Activating the high resolution system timer
> frequency gives us 1 ms wakeup precision.

Yeah, the number of bits we use to store this information is almost completely
unrelated to which timer we use on Windows...

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:541: random_number_ ^= (Now() -
base::TimeTicks()).InMilliseconds();
On 2017/06/26 21:48:12, gab wrote:
> This is a bitwise XOR, not sure what it means to mix bitness here,
> static_cast<uint32_t> the RHS to be safe?

Done.

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:938: 1000);
On 2017/06/26 21:48:12, gab wrote:
> Why is this changing?

There is no base::TimeTicks::FromMilliseconds.
I'd forgotten about the pattern to add a base::TimeDelta, I've switched to that.

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:349: const base::TimeDelta run_duration,
On 2017/06/26 21:48:12, gab wrote:
> const non-ref method params is weird, but I guess you're not changing that..

I can change this if you want - doesn't seem the style guide has anything to say
about it.

Let me know.

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:692: const base::TimeDelta queue_duration,
On 2017/06/26 21:48:12, gab wrote:
> const& ?
> 
> Either way, passing by value is preferred:
>
https://cs.chromium.org/chromium/src/base/time/time.h?sq=package:chromium&dr=...

Done.

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/2956683002/diff/20001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:657: const base::TimeTicks kTimePosted =
base::TimeTicks::FromInternalValue(1000);
On 2017/06/26 21:48:12, gab wrote:
> Why FromIntervalValue() here and elsewhere in this file?

Switched to base::TimeTicks() + base::TimeDelta::FromMilliseconds(1) throughout.

[tdresser, 2017-06-27 13:27:19]

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

[commit-bot: I haz the power, 2017-06-27 13:27:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 16:03:22]

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

[commit-bot: I haz the power, 2017-06-27 16:03:22]

Dry run: This issue passed the CQ dry run.

[gab, 2017-06-27 21:10:31]

lgtm

[tdresser, 2017-06-28 12:24:02]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2017-06-28 12:24:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdresser, 2017-06-28 12:24:18]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2017-06-28 12:24:49]

tdresser@chromium.org changed reviewers:
+ altimin@chromium.org

[tdresser, 2017-06-28 12:24:49]

+altimin for tiny change to
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc

[altimin, 2017-06-28 13:57:42]

lgtm with nit

https://codereview.chromium.org/2956683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2956683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:35:
base::TimeDelta::FromMilliseconds(duration.InMilliseconds()));
nit: FromMilliseconds is not needed anymore.

[tdresser, 2017-06-28 15:37:34]

https://codereview.chromium.org/2956683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2956683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:35:
base::TimeDelta::FromMilliseconds(duration.InMilliseconds()));
On 2017/06/28 13:57:42, altimin wrote:
> nit: FromMilliseconds is not needed anymore.

Whoops, thanks.

Done.

[tdresser, 2017-06-28 15:37:38]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2017-06-28 15:37:39]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
altimin@chromium.org
Link to the patchset: https://codereview.chromium.org/2956683002/#ps60001
(title: "Address nit.")

[commit-bot: I haz the power, 2017-06-28 15:37:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 17:53:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498664258651010,
"parent_rev": "a39debfb48a8b333cd989cfe1ff1649e39cd2fa7", "commit_rev":
"6f2691cc7756d599ce5622cb7c9fc854afcf3c57"}

[commit-bot: I haz the power, 2017-06-28 17:53:55]

Description was changed from

==========
chrome://profiler infrastructure uses base time types.

Previously, chrome://profiler avoided base::TimeTicks and
base::TimeDelta because they're 64 bits.

At this point, that appears to be premature optimization. Switch to
using the base time types.

This is required for using chrome://profiler's infrastructure for
building thread time UMA metrics.

BUG=736449
==========

to

==========
chrome://profiler infrastructure uses base time types.

Previously, chrome://profiler avoided base::TimeTicks and
base::TimeDelta because they're 64 bits.

At this point, that appears to be premature optimization. Switch to
using the base time types.

This is required for using chrome://profiler's infrastructure for
building thread time UMA metrics.

BUG=736449

Review-Url: https://codereview.chromium.org/2956683002
Cr-Commit-Position: refs/heads/master@{#483054}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f2691cc7756d599ce5622cb7c9f...
==========

[commit-bot: I haz the power, 2017-06-28 17:54:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6f2691cc7756d599ce5622cb7c9f...