New issue
Advanced search Search tips

Issue 616574 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

InterProcessTimeTicksConverter should only be used when TimeTicks are not consistent across processes

Project Member Reported by majidvp@chromium.org, Jun 1 2016

Issue description

InterProcessTimeTicksConverter is used to ensure we get a consistent monotonic tick when passing timeticks from one process to another. See [1,2] fore more information.

This utility is only needed when we don't have we don't have a monotonic clock that is consistent across processes. This should only be the case on Windows platform when we fallback to timeGetTime() as underlying ticks clock.

On all other platform this conversion is unnecessary and actually harmful because the conversion is an estimation and may introduce a skew when it is not necessary. 
So I propose that we make InterProcessTimeTicksConverter::Convert a no-op on platforms where timeticks is consistent across processes. 

[1] https://docs.google.com/document/d/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqsvM/edit
[2] go/timetiks
 
I wonder if it makes sense to add some method like "IsConsistentAcrossProcesses" to TimeTicks? That way, we could just have InterProcessTimeTicksConverter be a no-op if TimeTicks::IsConsistentAcrossProcesses() is true. It also seems like not a bad method to have just as a means of in-code documentation.
I was thinking of doing this when fixing this issue. At the moment this essentially maps to TimeTicks::IsHighResolution() but I think it makes sense for documentation purpose.

Owner: majidvp@chromium.org
Status: Started (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f3e28664ee4222ea54a442d996e40921a33b607

commit 4f3e28664ee4222ea54a442d996e40921a33b607
Author: majidvp <majidvp@chromium.org>
Date: Fri Jul 22 21:39:02 2016

Limit InterProcessTimeTickConverter to platforms that require it

On most platforms the monotonic clock is system wide which means it is
consistent across processes making conversion unnecessary. In fact not
only it is unnecessary but harmfull because the
InterProcessTimeTickConverter (IPTTC) conversion [1] is an estimation
which introduces some error.

The patch limits the usage of IPTTC to platforms that require it which
is any Windows where we cannot use |QueryPerformanceCounter|.

After this change we will only collect UMA metrics[2] for IPTTC on
platforms where it is used.

[1] https://docs.google.com/document/d/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqsvM/edit
[2] Affects InterProcessTimeTicks.*

BUG= 616574 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2090783002
Cr-Commit-Position: refs/heads/master@{#407270}

[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/base/time/time.h
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/base/time/time_mac.cc
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/base/time/time_posix.cc
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/base/time/time_win.cc
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/content/child/resource_dispatcher.cc
[modify] https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607/content/common/inter_process_time_ticks_converter.h

Is this bug okay to clos enow?
Status: Fixed (was: Started)

Sign in to add a comment