|
|
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. |
DescriptionLimit 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 #
Messages
Total messages: 32 (12 generated)
Description was changed from ========== Limit usage of InteProcessTimeTickConverter to platforms that need it BUG=616574 ========== to ========== Limit usage of InteProcessTimeTickConverter to platforms that need it BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Limit usage of InteProcessTimeTickConverter to platforms that need it BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Limit usage of InteProcessTimeTicksConverter 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 InterProcessTimeTicksConverter (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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Limit usage of InteProcessTimeTicksConverter 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 InterProcessTimeTicksConverter (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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Limit usage of InteProcessTimeTickConverter 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
majidvp@chromium.org changed reviewers: + charliea@chromium.org
On 2016/06/23 11:37:44, majidvp wrote: chariea@: friendly ping
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#newcod... base/time/time.h:712: // another. Time values from different threads that differ 1 tick have an A couple suggestions: 1) s/tick clock/TimeTicks 2) Add a comma after processes 3) s/tick counts/timestamps 4) I'd change that last sentence to something like: (Note that, even on platforms where this returns true, time values from different threads that are within one tick of each other must be considered to have an ambiguous ordering.) https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcod... base/time/time.h:714: static bool IsConsistentAcrossProcesses(); It looks like, in all cases, IsConsistentAcrossProcesses == IsHighResolution(). Should we just provide an implementation in time.cc?
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#newcod... base/time/time.h:712: // another. Time values from different threads that differ 1 tick have an On 2016/06/29 22:38:54, charliea wrote: > A couple suggestions: > > 1) s/tick clock/TimeTicks > 2) Add a comma after processes > 3) s/tick counts/timestamps > 4) I'd change that last sentence to something like: > > (Note that, even on platforms where this returns true, time values from > different threads that are within one tick of each other must be considered to > have an ambiguous ordering.) > Done. https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcod... base/time/time.h:714: static bool IsConsistentAcrossProcesses(); On 2016/06/29 22:38:54, charliea wrote: > It looks like, in all cases, IsConsistentAcrossProcesses == IsHighResolution(). > Should we just provide an implementation in time.cc? This is true but that should be seen as a "coincidence" not the rule. I am afraid if we move it into time.cc it may be seen as a rule and thus be a bit confusing. Given that this method's exists only to document the semantic of consistency across processes on different platforms I prefer to avoid any confusion specially that it won't buy us much if we move it up.
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#newcod... base/time/time.h:714: static bool IsConsistentAcrossProcesses(); On 2016/07/05 01:45:37, majidvp wrote: > On 2016/06/29 22:38:54, charliea wrote: > > It looks like, in all cases, IsConsistentAcrossProcesses == > IsHighResolution(). > > Should we just provide an implementation in time.cc? > > This is true but that should be seen as a "coincidence" not the rule. I am > afraid if we move it into time.cc it may be seen as a rule and thus be a bit > confusing. Given that this method's exists only to document the semantic of > consistency across processes on different platforms I prefer to avoid any > confusion specially that it won't buy us much if we move it up. > Acknowledged.
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): > > https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcod... > base/time/time.h:714: static bool IsConsistentAcrossProcesses(); > On 2016/07/05 01:45:37, majidvp wrote: > > On 2016/06/29 22:38:54, charliea wrote: > > > It looks like, in all cases, IsConsistentAcrossProcesses == > > IsHighResolution(). > > > Should we just provide an implementation in time.cc? > > > > This is true but that should be seen as a "coincidence" not the rule. I am > > afraid if we move it into time.cc it may be seen as a rule and thus be a bit > > confusing. Given that this method's exists only to document the semantic of > > consistency across processes on different platforms I prefer to avoid any > > confusion specially that it won't buy us much if we move it up. > > > > Acknowledged. (Withdrawing lg2m for discussion - not lgtm) What do you think about putting this check in ToLocalTimeTicks/ToLocalTimeDelta, and having them be no-ops if this is true? It seems like that might have better ergonomics than trying to have all callers check this first.
majidvp@chromium.org changed reviewers: + pfeldman@chromium.org
+ pfeldman@: For owner's review of content/
On 2016/07/06 17:48:17, charliea wrote: > 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): > > > > > https://codereview.chromium.org/2090783002/diff/20001/base/time/time.h#newcod... > > base/time/time.h:714: static bool IsConsistentAcrossProcesses(); > > On 2016/07/05 01:45:37, majidvp wrote: > > > On 2016/06/29 22:38:54, charliea wrote: > > > > It looks like, in all cases, IsConsistentAcrossProcesses == > > > IsHighResolution(). > > > > Should we just provide an implementation in time.cc? > > > > > > This is true but that should be seen as a "coincidence" not the rule. I am > > > afraid if we move it into time.cc it may be seen as a rule and thus be a bit > > > confusing. Given that this method's exists only to document the semantic of > > > consistency across processes on different platforms I prefer to avoid any > > > confusion specially that it won't buy us much if we move it up. > > > > > > > Acknowledged. > > (Withdrawing lg2m for discussion - not lgtm) > > What do you think about putting this check in ToLocalTimeTicks/ToLocalTimeDelta, > and having them be no-ops if this is true? It seems like that might have better > ergonomics than trying to have all callers check this first. I did originally think about doing that. I considered it not a good idea mainly due to two issues: 1- Initializing the converter needs 4 times stamps (remote & local lower and upper bounds) Having the check outside means that one does not need to have these bounds on platforms that don't need it. 2- I also wanted to skip UMA metrics if the conversion is skipped. To address (2) I probably can move UMA collections inside converter (maybe in dtor?) (1) is not really an issue in practice because we always have those bounds I just found it a bit odd in terms of the API but if that is fine with you I have no objection. The problem was that we also want to skip UMA reporting if that is the case:
Those seem like reasonable arguments to me, combined with the fact that this matches closely the way that things are already done. non-owner lgtm - I'd recommend trying to reach out to someone with some timing experience (possibly thestig@), for an owner review
majidvp@chromium.org changed reviewers: + thestig@chromium.org
+ thestig@ for review.
Description was changed from ========== Limit usage of InteProcessTimeTickConverter 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
+fdoray FYI if there's any comments I fixed some CL description typos / formatting. base/ lgtm 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#n... base/time/time_win.cc:508: // According to Windows documentation [1] QPC is consistent Post-Windows Visa. Visa -> Vista?
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
Changes to base/time/ LGTM
On 2016/07/07 20:59:28, fdoray wrote: > Changes to base/time/ LGTM Thanks pfeldman@: friendly ping!
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#n... base/time/time_win.cc:508: // According to Windows documentation [1] QPC is consistent Post-Windows Visa. On 2016/07/07 20:36:30, Lei Zhang wrote: > Visa -> Vista? Done.
lgtm
The CQ bit was checked by majidvp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, fdoray@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2090783002/#ps60001 (title: "Minor comment fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is deprecated: tryserver.chromium.linux For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from ========== 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [2] Affects InterProcessTimeTicks.* BUG=616574 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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/1RcaOZ1G8ttAez9YuilvbDJCkCaYuLKqNkVpQvIBqs... [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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4f3e28664ee4222ea54a442d996e40921a33b607 Cr-Commit-Position: refs/heads/master@{#407270} |