Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(157)

Issue 2090783002: Limit InterProcessTimeTicksConverter to platforms that require it (Closed)

Created:
4 years, 6 months ago by majidvp
Modified:
4 years, 5 months ago
CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, ppi, fdoray
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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 Committed: https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607 Cr-Commit-Position: refs/heads/master@{#407270}

Patch Set 1 #

Patch Set 2 : Fix mac #

Total comments: 5

Patch Set 3 : Address feedback #

Total comments: 2

Patch Set 4 : Minor comment fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -31 lines) Patch
M base/time/time.h View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M base/time/time_mac.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M base/time/time_posix.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M base/time/time_win.cc View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 chunk +32 lines, -28 lines 0 comments Download
M content/child/resource_dispatcher.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/common/inter_process_time_ticks_converter.h View 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 32 (12 generated)
majidvp
4 years, 6 months ago (2016-06-23 11:37:44 UTC) #5
majidvp
On 2016/06/23 11:37:44, majidvp wrote: chariea@: friendly ping
4 years, 5 months ago (2016-06-27 15:04:31 UTC) #6
charliea (OOO until 10-5)
https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcode712 base/time/time.h:712: // another. Time values from different threads that differ ...
4 years, 5 months ago (2016-06-29 22:38:54 UTC) #7
majidvp
https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcode712 base/time/time.h:712: // another. Time values from different threads that differ ...
4 years, 5 months ago (2016-07-05 01:45:37 UTC) #8
charliea (OOO until 10-5)
non-owner lgtm https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcode714 base/time/time.h:714: static bool IsConsistentAcrossProcesses(); On 2016/07/05 01:45:37, majidvp ...
4 years, 5 months ago (2016-07-06 17:43:52 UTC) #9
charliea (OOO until 10-5)
On 2016/07/06 17:43:52, charliea wrote: > non-owner lgtm > > https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h > File base/time/time.h (right): ...
4 years, 5 months ago (2016-07-06 17:48:17 UTC) #10
majidvp
+ pfeldman@: For owner's review of content/
4 years, 5 months ago (2016-07-06 17:54:29 UTC) #12
majidvp
On 2016/07/06 17:48:17, charliea wrote: > On 2016/07/06 17:43:52, charliea wrote: > > non-owner lgtm ...
4 years, 5 months ago (2016-07-06 18:07:30 UTC) #13
charliea (OOO until 10-5)
Those seem like reasonable arguments to me, combined with the fact that this matches closely ...
4 years, 5 months ago (2016-07-07 19:34:38 UTC) #14
majidvp
+ thestig@ for review.
4 years, 5 months ago (2016-07-07 19:40:19 UTC) #16
Lei Zhang
+fdoray FYI if there's any comments I fixed some CL description typos / formatting. base/ ...
4 years, 5 months ago (2016-07-07 20:36:30 UTC) #18
fdoray
Changes to base/time/ LGTM
4 years, 5 months ago (2016-07-07 20:59:28 UTC) #20
majidvp
On 2016/07/07 20:59:28, fdoray wrote: > Changes to base/time/ LGTM Thanks pfeldman@: friendly ping!
4 years, 5 months ago (2016-07-08 14:49:10 UTC) #21
majidvp
https://codereview.chromium.org/2090783002/diff/40001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/2090783002/diff/40001/base/time/time_win.cc#newcode508 base/time/time_win.cc:508: // According to Windows documentation [1] QPC is consistent ...
4 years, 5 months ago (2016-07-08 15:38:22 UTC) #22
majidvp
4 years, 5 months ago (2016-07-08 15:38:26 UTC) #23
pfeldman
lgtm
4 years, 5 months ago (2016-07-22 18:08:55 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2090783002/60001
4 years, 5 months ago (2016-07-22 18:11:01 UTC) #27
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 5 months ago (2016-07-22 18:11:03 UTC) #28
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 5 months ago (2016-07-22 21:39:18 UTC) #30
commit-bot: I haz the power
4 years, 5 months ago (2016-07-22 21:40:26 UTC) #32
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607
Cr-Commit-Position: refs/heads/master@{#407270}

Powered by Google App Engine
This is Rietveld 408576698