Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775663002:
Replace TimeTicks::UnixEpoch with Time::UnixEpoch for the net log (Closed)
|
Created:
4 years, 9 months ago by Charlie Harrison Modified:
4 years, 9 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace TimeTicks::UnixEpoch with Time::UnixEpoch for the net log
This patch replaces the value of timeTicksOffset for the net log with a value
calculated via:
(Time::Now() - Time::UnixEpoch()) - (TimeTicks::Now() - TimeTicks())
In JS, TimeTicks values calculated via (t - TimeTicks()) are added to the
timeTicksOffset to calculate the wall time that they occurred.
This is equivalent to passing the TimeDelta since the epoch, along with a
reference monotonic time for subsequent TimeTicks values to subtract off.
BUG=592107
Committed: https://crrev.com/796645a3cfdf37d6f9bde8ba0af644a8950df0cc
Cr-Commit-Position: refs/heads/master@{#380725}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated comment to be scarier and reference crbug #
Total comments: 3
Messages
Total messages: 33 (7 generated)
csharrison@chromium.org changed reviewers: + charliea@chromium.org, mmenke@chromium.org
mmenke@, charliea@, ptal. This is maybe not the best implementation, but it's simpler than changing all the javascript that uses this value. Matt, afaict, this is always created when the logging starts, right? This is a better reference Time than using UnixEpoch's, and we'll only get the suspend skew if the suspend occurs during logging.
mmenke@chromium.org changed reviewers: + eroman@chromium.org
[+eroman]: Fun times. I'll think about this tomorrow.
lgtm https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc#new... net/log/net_log_util.cc:279: // milliseconds by subtracting the null TimeTicks. I am not sure what this comment means. Subtracting null TimeTicks from a TimeTicks leaves the result unchanged, since it is TimeTicks(0) no? (Just changes the type).
https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc#new... net/log/net_log_util.cc:279: // milliseconds by subtracting the null TimeTicks. On 2016/03/08 at 00:11:04, eroman wrote: > I am not sure what this comment means. Subtracting null TimeTicks from a TimeTicks leaves the result unchanged, since it is TimeTicks(0) no? (Just changes the type). Yeah that's what I meant. I thought saying something like "comparable to TimeTicks" would be a bit imprecise. Maybe"comparable to TimeTicks in milliseconds" is better.
https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc#new... net/log/net_log_util.cc:289: constants_dict->SetString("timeTickOffset", Am I right in that we're just trying to do a drop-in replacement here so that we can eventually kill TimeTicks::UnixEpoch()? If so, can you add a scary comment above indicating that we know the operation we're doing here is flawed, and explain why? Something like: TODO(csharrison): This code computes the TimeDelta that must be subtracted from a TimeTicks value in order for that TimeTicks to be relative to the Unix epoch rather than some arbitrary point in the past (usually the system boot time). However, the amount that must be subtracted is inherently subject to change. TimeTicks takes leap seconds into account, whereas Unix time does not. On some systems, TimeTicks stops when the computer is asleep, whereas Unix time does not. Because of this, correctly storing this offset as a constant is impossible.
https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/1/net/log/net_log_util.cc#new... net/log/net_log_util.cc:289: constants_dict->SetString("timeTickOffset", On 2016/03/08 at 17:01:42, charliea wrote: > Am I right in that we're just trying to do a drop-in replacement here so that we can eventually kill TimeTicks::UnixEpoch()? If so, can you add a scary comment above indicating that we know the operation we're doing here is flawed, and explain why? > > Something like: > > TODO(csharrison): This code computes the TimeDelta that must be subtracted from a TimeTicks value in order for that TimeTicks to be relative to the Unix epoch rather than some arbitrary point in the past (usually the system boot time). However, the amount that must be subtracted is inherently subject to change. TimeTicks takes leap seconds into account, whereas Unix time does not. On some systems, TimeTicks stops when the computer is asleep, whereas Unix time does not. Because of this, correctly storing this offset as a constant is impossible. I'm not sure why this operation is as flawed as you say. Events are stored with TimeTicks timestamps, because we care about event monotonicity, etc. This reference time is just used for conversion for UI purposes, and it will only be incorrect for skew introduced during logging (the constant is created when logging starts). This mimics blink's behavior to generate reference monotonic and wall time values for performance.timing. I think the only improvement on this would be to log every event with a TimeTicks timestamp and a Time ui_timestamp. Is that what you are advocating?
On 2016/03/08 17:13:41, csharrison wrote:
> I'm not sure why this operation is as flawed as you say. Events are stored
with
> TimeTicks timestamps, because we care about event monotonicity, etc. This
> reference time is just used for conversion for UI purposes, and it will only
be
> incorrect for skew introduced during logging (the constant is created when
> logging starts).
A problem here might be that I don't have much knowledge about the code that
uses this. How long is it generally between the time that the logging constants
are initialized and the time that the code uses the constants? How important is
it that the unix timestamps be even reasonably correct? If the logging constant
get initialized, a user puts their computer to sleep for 2 days, then wakes the
computer up, and uses the logging constant, the unix time that's generated will
be incorrect by 2 days. Is that acceptable? It might be - I guess I just don't
have enough context.
> This mimics blink's behavior to generate reference monotonic and wall time
> values for performance.timing.
I've only seen performance.timing spit out non-human-readable timestamps, like
(in the JS console):
> window.performance.mark('mark1')
> window.performance.mark('mark2')
> window.performance.measure('measure1', 'mark1', 'mark2')
> window.performance.getEntriesByName('mark1')
[PerformanceMark] duration: 0, entryType: "mark", name: "mark1", startTime:
70497.46
> window.performance.getEntriesByName('measure1')
[PerformanceMeasure] duration: 6226.774999999994, entryType: "measure", name:
"measure1", startTime: 70497.46
It looks like the timestamps are relative to the time that the page was opened,
which I've always assumed was stored as a TimeTicks. Is that wrong?
> I think the only improvement on this would be to log every event with a
> TimeTicks timestamp and a Time ui_timestamp. Is that what you are advocating?
Nat and I have actually discussed this. It probably isn't a reasonable solution
to the problem (work-wise or memory-wise), but it's probably the only real one.
I'm not necessarily advocating fixing the problem - that's probably way outside
the scope of this CL - but I think documenting the problems with the current
code is important.
On 2016/03/08 at 19:21:39, charliea wrote:
> On 2016/03/08 17:13:41, csharrison wrote:
> > I'm not sure why this operation is as flawed as you say. Events are stored
with
> > TimeTicks timestamps, because we care about event monotonicity, etc. This
> > reference time is just used for conversion for UI purposes, and it will only
be
> > incorrect for skew introduced during logging (the constant is created when
> > logging starts).
>
> A problem here might be that I don't have much knowledge about the code that
uses this. How long is it generally between the time that the logging constants
are initialized and the time that the code uses the constants? How important is
it that the unix timestamps be even reasonably correct? If the logging constant
get initialized, a user puts their computer to sleep for 2 days, then wakes the
computer up, and uses the logging constant, the unix time that's generated will
be incorrect by 2 days. Is that acceptable? It might be - I guess I just don't
have enough context.
It's my understanding that these constants are generated at the start of
logging, and logging is usually a short lived ordeal. mmenke@, please correct me
if I'm wrong about the expected use cases of the net log. I think giving bad
timestamps if the user suspends during net logging is reasonable, though.
>
> > This mimics blink's behavior to generate reference monotonic and wall time
> > values for performance.timing.
>
> I've only seen performance.timing spit out non-human-readable timestamps, like
(in the JS console):
>
> > window.performance.mark('mark1')
> > window.performance.mark('mark2')
> > window.performance.measure('measure1', 'mark1', 'mark2')
> > window.performance.getEntriesByName('mark1')
> [PerformanceMark] duration: 0, entryType: "mark", name: "mark1", startTime:
70497.46
> > window.performance.getEntriesByName('measure1')
> [PerformanceMeasure] duration: 6226.774999999994, entryType: "measure", name:
"measure1", startTime: 70497.46
>
> It looks like the timestamps are relative to the time that the page was
opened, which I've always assumed was stored as a TimeTicks. Is that wrong?
Sorry I should have been more clear. I meant the values in performance.timing
like performance.timing.loadEventEnd, which is converted from TimeTicks to wall
time by way of a reference Time value created at navigation start. To be clear,
this is a horrible API and the new features that you've mentioned do away with
this problem.
>
> > I think the only improvement on this would be to log every event with a
> > TimeTicks timestamp and a Time ui_timestamp. Is that what you are
advocating?
>
> Nat and I have actually discussed this. It probably isn't a reasonable
solution to the problem (work-wise or memory-wise), but it's probably the only
real one. I'm not necessarily advocating fixing the problem - that's probably
way outside the scope of this CL - but I think documenting the problems with the
current code is important.
Okay yeah I agree. Thanks for the review :)
I am fine with this approach, as it helps remove a dependency on TimeTicks::UnixEpoch. As csharrison described, the current use case in NetLog is to estimate the timestamp for network events in logs. It doesn't need to be perfect, but the walltime helps correlate events with other sources of data from the system (like packet captures), or with user comments. The current implementation is already broken as described by this thread: * TimeTicks::UnixEpoch being currently used doesn't work across suspends, so this change doesn't make things any worse (in fact reduces the scope of this badness) * TimeTicks has different behaviors for each platform. i.e. whether it continues ticking during suspends differs on Windows from Linux. This is kind of annoying, and would be nice to have strong guarantees from TimeTicks in the future. I could think of more sophisticated approaches like monitoring when suspend state was entered, or system clock changes, but honestly I don't think fully fixing this is necessary, or warrants the extra complexity. I think a reasonable compromise could be to sample this ticks to walltime at two points: when starting logging, and when ending logging. The log loading code could then tell if there was a timing anomaly that happened during logging and warn that the timestamps are weird.
On 2016/03/08 at 21:57:34, eroman wrote: > I am fine with this approach, as it helps remove a dependency on TimeTicks::UnixEpoch. > > As csharrison described, the current use case in NetLog is to estimate the timestamp for network events in logs. > > It doesn't need to be perfect, but the walltime helps correlate events with other sources of data from the system (like packet captures), or with user comments. > > The current implementation is already broken as described by this thread: > > * TimeTicks::UnixEpoch being currently used doesn't work across suspends, so this change doesn't make things any worse (in fact reduces the scope of this badness) > > * TimeTicks has different behaviors for each platform. i.e. whether it continues ticking during suspends differs on Windows from Linux. This is kind of annoying, and would be nice to have strong guarantees from TimeTicks in the future. > > I could think of more sophisticated approaches like monitoring when suspend state was entered, or system clock changes, but honestly I don't think fully fixing this is necessary, or warrants the extra complexity. > > I think a reasonable compromise could be to sample this ticks to walltime at two points: when starting logging, and when ending logging. The log loading code could then tell if there was a timing anomaly that happened during logging and warn that the timestamps are weird. This suggestion SGTM. I opened 593157 to implement this feature (adding a warning to the UI).
On 2016/03/08 22:54:52, csharrison wrote: > On 2016/03/08 at 21:57:34, eroman wrote: > > I am fine with this approach, as it helps remove a dependency on > TimeTicks::UnixEpoch. > > > > As csharrison described, the current use case in NetLog is to estimate the > timestamp for network events in logs. > > > > It doesn't need to be perfect, but the walltime helps correlate events with > other sources of data from the system (like packet captures), or with user > comments. > > > > The current implementation is already broken as described by this thread: > > > > * TimeTicks::UnixEpoch being currently used doesn't work across suspends, so > this change doesn't make things any worse (in fact reduces the scope of this > badness) > > > > * TimeTicks has different behaviors for each platform. i.e. whether it > continues ticking during suspends differs on Windows from Linux. This is kind > of annoying, and would be nice to have strong guarantees from TimeTicks in the > future. > > > > I could think of more sophisticated approaches like monitoring when suspend > state was entered, or system clock changes, but honestly I don't think fully > fixing this is necessary, or warrants the extra complexity. > > > > I think a reasonable compromise could be to sample this ticks to walltime at > two points: when starting logging, and when ending logging. The log loading code > could then tell if there was a timing anomaly that happened during logging and > warn that the timestamps are weird. > > This suggestion SGTM. I opened 593157 to implement this feature (adding a > warning to the UI). Deferring to eroman on this CL (Exchange triage shifts, don't want to spend time thinking about this, and a bit behind on my triage duties)
charliea@, want to take a look at the updated comment?
https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); This seems to completely defeat the purpose of using TimeTicks to get more accurate times in the first place...No? If we're going to do this, we should switch to Time everywhere instead...Or use a constant delta between Time and TimeTicks, unless there's a sudden jump.
https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); On 2016/03/10 at 22:19:35, mmenke wrote: > This seems to completely defeat the purpose of using TimeTicks to get more accurate times in the first place...No? If we're going to do this, we should switch to Time everywhere instead...Or use a constant delta between Time and TimeTicks, unless there's a sudden jump. You have a point, but I assumed that we needed TimeTicks in the netlog internally to properly order event / make the waterfall, etc. If a timing anomaly occurred, it would make things extremely hard to reason about if events started displaying out of order. My assumption might be wrong though, I don't know the netlog code very well.
On 2016/03/10 22:22:25, csharrison wrote: > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc > File net/log/net_log_util.cc (right): > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... > net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); > On 2016/03/10 at 22:19:35, mmenke wrote: > > This seems to completely defeat the purpose of using TimeTicks to get more > accurate times in the first place...No? If we're going to do this, we should > switch to Time everywhere instead...Or use a constant delta between Time and > TimeTicks, unless there's a sudden jump. > > You have a point, but I assumed that we needed TimeTicks in the netlog > internally to properly order event / make the waterfall, etc. If a timing > anomaly occurred, it would make things extremely hard to reason about if events > started displaying out of order. > > My assumption might be wrong though, I don't know the netlog code very well. net-internals usually orders by even ID, actually...TimeTicks are used just to get more accurate timing, I believe.
On 2016/03/10 at 22:39:46, mmenke wrote: > On 2016/03/10 22:22:25, csharrison wrote: > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc > > File net/log/net_log_util.cc (right): > > > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... > > net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); > > On 2016/03/10 at 22:19:35, mmenke wrote: > > > This seems to completely defeat the purpose of using TimeTicks to get more > > accurate times in the first place...No? If we're going to do this, we should > > switch to Time everywhere instead...Or use a constant delta between Time and > > TimeTicks, unless there's a sudden jump. > > > > You have a point, but I assumed that we needed TimeTicks in the netlog > > internally to properly order event / make the waterfall, etc. If a timing > > anomaly occurred, it would make things extremely hard to reason about if events > > started displaying out of order. > > > > My assumption might be wrong though, I don't know the netlog code very well. > > net-internals usually orders by even ID, actually...TimeTicks are used just to get more accurate timing, I believe. If TimeTicks are only used to show a timestamp to the user, let's switch it out to use Time instead.
https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc File net/log/net_log_util.cc (right): https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); On 2016/03/10 22:22:25, csharrison wrote: > On 2016/03/10 at 22:19:35, mmenke wrote: > > This seems to completely defeat the purpose of using TimeTicks to get more > accurate times in the first place...No? If we're going to do this, we should > switch to Time everywhere instead...Or use a constant delta between Time and > TimeTicks, unless there's a sudden jump. > > You have a point, but I assumed that we needed TimeTicks in the netlog > internally to properly order event / make the waterfall, etc. If a timing > anomaly occurred, it would make things extremely hard to reason about if events > started displaying out of order. > > My assumption might be wrong though, I don't know the netlog code very well. I disagree that using Time everywhere is equivalent. One of my concerns with Time is that it is subject to change at any moment, and fundamentally seems ill-suited for measuring elapsed time deltas (I have similar concerns on its liberal use throughout Chrome). A standard configuration these days is for your computer to pull time information from the network and automatically adjust its system clock.... So we aren't just talking about manual interventions where the user goes and changes their system time so they can play candy crush levels, but also small (or large) corrections to Time that happen during the normal course of operation. It seems wrong for something measuring network timings to carry a dependency on the network itself (i.e. NTP). I don't think this is just an academic concern: I have a crufy workstation at home where my CMOS battery is presumably dead. Since each time I power it on the clock is way off. But after a couple minutes of operation (why so long I don't know...) it sets itself back to something reasonable. Unless there is a good reason, I would say stick with TimeTicks.
On 2016/03/10 at 23:18:08, eroman wrote: > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc > File net/log/net_log_util.cc (right): > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... > net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); > On 2016/03/10 22:22:25, csharrison wrote: > > On 2016/03/10 at 22:19:35, mmenke wrote: > > > This seems to completely defeat the purpose of using TimeTicks to get more > > accurate times in the first place...No? If we're going to do this, we should > > switch to Time everywhere instead...Or use a constant delta between Time and > > TimeTicks, unless there's a sudden jump. > > > > You have a point, but I assumed that we needed TimeTicks in the netlog > > internally to properly order event / make the waterfall, etc. If a timing > > anomaly occurred, it would make things extremely hard to reason about if events > > started displaying out of order. > > > > My assumption might be wrong though, I don't know the netlog code very well. > > I disagree that using Time everywhere is equivalent. > > One of my concerns with Time is that it is subject to change at any moment, and fundamentally seems ill-suited for measuring elapsed time deltas (I have similar concerns on its liberal use throughout Chrome). > > A standard configuration these days is for your computer to pull time information from the network and automatically adjust its system clock.... So we aren't just talking about manual interventions where the user goes and changes their system time so they can play candy crush levels, but also small (or large) corrections to Time that happen during the normal course of operation. > > It seems wrong for something measuring network timings to carry a dependency on the network itself (i.e. NTP). > > I don't think this is just an academic concern: I have a crufy workstation at home where my CMOS battery is presumably dead. Since each time I power it on the clock is way off. But after a couple minutes of operation (why so long I don't know...) it sets itself back to something reasonable. > > Unless there is a good reason, I would say stick with TimeTicks. The question is, what is the purpose of these timestamps? Which purposes are the most vital. Two major ones are: 1. Delta between timestamps show accurate duration between events. 2. Timestamps reflect system time so they can be correlated with outside sources, even if system time changes. This change gives us (1), and drastically improves (2) by setting the reference time to log creation instead of Chrome initialization. Changing to Time everywhere gives us (2) without (1).
On 2016/03/11 15:37:33, csharrison wrote: > On 2016/03/10 at 23:18:08, eroman wrote: > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc > > File net/log/net_log_util.cc (right): > > > > > https://codereview.chromium.org/1775663002/diff/20001/net/log/net_log_util.cc... > > net/log/net_log_util.cc:289: base::TimeTicks::Now() - base::TimeTicks(); > > On 2016/03/10 22:22:25, csharrison wrote: > > > On 2016/03/10 at 22:19:35, mmenke wrote: > > > > This seems to completely defeat the purpose of using TimeTicks to get more > > > accurate times in the first place...No? If we're going to do this, we > should > > > switch to Time everywhere instead...Or use a constant delta between Time and > > > TimeTicks, unless there's a sudden jump. > > > > > > You have a point, but I assumed that we needed TimeTicks in the netlog > > > internally to properly order event / make the waterfall, etc. If a timing > > > anomaly occurred, it would make things extremely hard to reason about if > events > > > started displaying out of order. > > > > > > My assumption might be wrong though, I don't know the netlog code very well. > > > > I disagree that using Time everywhere is equivalent. > > > > One of my concerns with Time is that it is subject to change at any moment, > and fundamentally seems ill-suited for measuring elapsed time deltas (I have > similar concerns on its liberal use throughout Chrome). > > > > A standard configuration these days is for your computer to pull time > information from the network and automatically adjust its system clock.... So we > aren't just talking about manual interventions where the user goes and changes > their system time so they can play candy crush levels, but also small (or large) > corrections to Time that happen during the normal course of operation. > > > > It seems wrong for something measuring network timings to carry a dependency > on the network itself (i.e. NTP). > > > > I don't think this is just an academic concern: I have a crufy workstation at > home where my CMOS battery is presumably dead. Since each time I power it on the > clock is way off. But after a couple minutes of operation (why so long I don't > know...) it sets itself back to something reasonable. > > > > Unless there is a good reason, I would say stick with TimeTicks. > > The question is, what is the purpose of these timestamps? Which purposes are the > most vital. Two major ones are: > 1. Delta between timestamps show accurate duration between events. > 2. Timestamps reflect system time so they can be correlated with outside > sources, even if system time changes. > > This change gives us (1), and drastically improves (2) by setting the reference > time to log creation instead of Chrome initialization. Changing to Time > everywhere gives us (2) without (1). Sorry, I was thinking the code was in the per-logged-event code, rather than the constants code. I thought we switched to using a timeTickOffset off 0 at some point, so didn't realize what was really going on. This solution seems reasonable, then. LGTM.
> The question is, what is the purpose of these timestamps? Which purposes are the > most vital. Two major ones are: > 1. Delta between timestamps show accurate duration between events. > 2. Timestamps reflect system time so they can be correlated with outside > sources, even if system time changes. > > This change gives us (1), and drastically improves (2) by setting the reference > time to log creation instead of Chrome initialization. Changing to Time > everywhere gives us (2) without (1). Agreed. I think there's some miscommunication happening: I think that everyone agrees that Time is meant for accurate user-readable timestamps, and TimeTicks is meant for more accurate deltas. (FWIW, TimeTicks is still subject to NTP, but only to clock slewing, not large instantaneous clock adjustments.) It sounds like the real problem though is that we don't have the context to know which use case this number is being used for. My opinion is for lgtm (as a not-at-all-a-network-domain-expert): the patch makes the code strictly better. If someone wants to come along later and make it even better by switching to Time where appropriate, that's still an option.
On 2016/03/11 at 18:15:27, charliea wrote: > > The question is, what is the purpose of these timestamps? Which purposes are the > > most vital. Two major ones are: > > 1. Delta between timestamps show accurate duration between events. > > 2. Timestamps reflect system time so they can be correlated with outside > > sources, even if system time changes. > > > > This change gives us (1), and drastically improves (2) by setting the reference > > time to log creation instead of Chrome initialization. Changing to Time > > everywhere gives us (2) without (1). > > Agreed. I think there's some miscommunication happening: I think that everyone agrees that Time is meant for accurate user-readable timestamps, and TimeTicks is meant for more accurate deltas. (FWIW, TimeTicks is still subject to NTP, but only to clock slewing, not large instantaneous clock adjustments.) It sounds like the real problem though is that we don't have the context to know which use case this number is being used for. > > My opinion is for lgtm (as a not-at-all-a-network-domain-expert): the patch makes the code strictly better. If someone wants to come along later and make it even better by switching to Time where appropriate, that's still an option. Sounds good to me. Thanks everyone!
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1775663002/#ps20001 (title: "Updated comment to be scarier and reference crbug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775663002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775663002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775663002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775663002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Replace TimeTicks::UnixEpoch with Time::UnixEpoch for the net log This patch replaces the value of timeTicksOffset for the net log with a value calculated via: (Time::Now() - Time::UnixEpoch()) - (TimeTicks::Now() - TimeTicks()) In JS, TimeTicks values calculated via (t - TimeTicks()) are added to the timeTicksOffset to calculate the wall time that they occurred. This is equivalent to passing the TimeDelta since the epoch, along with a reference monotonic time for subsequent TimeTicks values to subtract off. BUG=592107 ========== to ========== Replace TimeTicks::UnixEpoch with Time::UnixEpoch for the net log This patch replaces the value of timeTicksOffset for the net log with a value calculated via: (Time::Now() - Time::UnixEpoch()) - (TimeTicks::Now() - TimeTicks()) In JS, TimeTicks values calculated via (t - TimeTicks()) are added to the timeTicksOffset to calculate the wall time that they occurred. This is equivalent to passing the TimeDelta since the epoch, along with a reference monotonic time for subsequent TimeTicks values to subtract off. BUG=592107 Committed: https://crrev.com/796645a3cfdf37d6f9bde8ba0af644a8950df0cc Cr-Commit-Position: refs/heads/master@{#380725} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/796645a3cfdf37d6f9bde8ba0af644a8950df0cc Cr-Commit-Position: refs/heads/master@{#380725} |
