Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472163003:
[CustomTabs] Report the navigation start as absolute time (Closed)
|
Created:
4 years, 1 month ago by droger Modified:
4 years, 1 month ago CC:
agrieve+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, lizeb+watch-custom-tabs_chromium.org, loading-reviews+metrics_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[CustomTabs] Report the navigation start as absolute time
This CL adds the date of the navigation start (in milliseconds since
epoch) to the page load metrics callback.
This allows the client application to convert the timings into absolute
times.
Committed: https://crrev.com/bfa430e1a59cfac9230f836b164621beecba3f02
Cr-Commit-Position: refs/heads/master@{#433507}
Patch Set 1 #Patch Set 2 : Test #Patch Set 3 : 80 columns #Patch Set 4 : One more test for ttfcp #
Total comments: 2
Patch Set 5 : Use TimeTicks #Patch Set 6 : Comments #
Total comments: 2
Patch Set 7 : Compute time offset #
Total comments: 7
Patch Set 8 : Rebase + comments #
Total comments: 12
Patch Set 9 : Review comments #Patch Set 10 : Review comments #Patch Set 11 : Cache tick offset #
Total comments: 6
Messages
Total messages: 80 (38 generated)
droger@chromium.org changed reviewers: + lizeb@chromium.org
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added a test.
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/04 17:26:19, droger wrote: > Added a test. LGTM, thanks!
droger@chromium.org changed reviewers: + bauerb@chromium.org, bmcquade@chromium.org
+bauerb for PageLoadMetrics.java +bmcquade for android_page_load_metrics_observer.cc
droger@chromium.org changed reviewers: + miguelg@chromium.org - bauerb@chromium.org
-bauerb (OOO) + miguelg for PageLoadMetrics.java
bmcquade, miguelg: ping
droger@chromium.org changed reviewers: + bauerb@chromium.org, csharrison@chromium.org
Adding bauerb, csharrison since miguelg and bmcquade are not responding. bauerb: can you OWNER review PageLoadMetrics.java ? csharrison: can you OWNER review android_page_load_metrics_observer.cc
Could you add a BUG= field to the CL? Thanks! https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: long current = System.currentTimeMillis(); Using a real clock here makes me queasy. In addition, do we even know that System.currentTimeMillis() uses the same underlying clock as what the navigation uses? I think I've traced it to base::TimeTicks::Now() (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), which is monotonous, whereas System.currentTimeMillis() seems to return wall clock time...? Would it work to use a time here that is guaranteed to be in the future, and below a time that is guaranteed to be in the past?
Thanks for the comments. https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: long current = System.currentTimeMillis(); On 2016/11/14 10:27:36, Bernhard Bauer wrote: > Using a real clock here makes me queasy. In addition, do we even know that > System.currentTimeMillis() uses the same underlying clock as what the navigation > uses? I think I've traced it to base::TimeTicks::Now() > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > which is monotonous, whereas System.currentTimeMillis() seems to return wall > clock time...? > This time comes from android_page_load_metrics_observer.cc. See in particular the call to ToJavaTime(). I believe these times are comparable. I'm using this wall clock, since I need some kind of time that can be used by another application. Note that this file is a test for a functionality exposed to other Android apps. I would be ok to use another clock, but note that I cannot use base::Time or some Chromium structure. I need to use Android APIs. > Would it work to use a time here that is guaranteed to be in the future, and > below a time that is guaranteed to be in the past? I don't really understand the concern. Are you afraid of rounding errors?
On 2016/11/14 at 10:39:19, droger wrote: > Thanks for the comments. > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: long current = System.currentTimeMillis(); > On 2016/11/14 10:27:36, Bernhard Bauer wrote: > > Using a real clock here makes me queasy. In addition, do we even know that > > System.currentTimeMillis() uses the same underlying clock as what the navigation > > uses? I think I've traced it to base::TimeTicks::Now() > > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > > which is monotonous, whereas System.currentTimeMillis() seems to return wall > > clock time...? > > > > This time comes from android_page_load_metrics_observer.cc. See in particular the call to ToJavaTime(). I believe these times are comparable. > > I'm using this wall clock, since I need some kind of time that can be used by another application. Note that this file is a test for a functionality exposed to other Android apps. > I would be ok to use another clock, but note that I cannot use base::Time or some Chromium structure. I need to use Android APIs. > > > Would it work to use a time here that is guaranteed to be in the future, and > > below a time that is guaranteed to be in the past? > > I don't really understand the concern. Are you afraid of rounding errors? We're actually trying to get away from using base::Time since it isn't monotonic and can lead to significant errors if the epoch-based clock is changed between taking two base::Times that are used to compute a duration. We should be using TimeTicks wherever we can. Is a value from TimeTicks something that could be used here instead? It looks like Java-based code should be able to get a TimeTicks equivalent using https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil.... To provide the TimeTicks-based nav start time to clients, you can get the TimeTicks-based nav start from the NavigationHandle in OnStart or OnCommit in android_page_load_metrics_observer.cc.
On 2016/11/14 15:02:39, Bryan McQuade wrote: > On 2016/11/14 at 10:39:19, droger wrote: > > Thanks for the comments. > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > File > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > (right): > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: > long current = System.currentTimeMillis(); > > On 2016/11/14 10:27:36, Bernhard Bauer wrote: > > > Using a real clock here makes me queasy. In addition, do we even know that > > > System.currentTimeMillis() uses the same underlying clock as what the > navigation > > > uses? I think I've traced it to base::TimeTicks::Now() > > > > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > > > which is monotonous, whereas System.currentTimeMillis() seems to return wall > > > clock time...? > > > > > > > This time comes from android_page_load_metrics_observer.cc. See in particular > the call to ToJavaTime(). I believe these times are comparable. The thing is, AndroidPageLoadMetricsObserver gets the value from PageLoadTiming, which (via some other hops) gets it from the NavigationParams, which get it from TimeTicks. blink::PerformanceTiming does in fact convert from TimeTicks to epoch-based time (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...), so the epoch and scale is going to match, but the value is still going to be monotonic, whereas wall clock time is not. > > I'm using this wall clock, since I need some kind of time that can be used by > another application. Note that this file is a test for a functionality exposed > to other Android apps. > > I would be ok to use another clock, but note that I cannot use base::Time or > some Chromium structure. I need to use Android APIs. > > > > > Would it work to use a time here that is guaranteed to be in the future, and > > > below a time that is guaranteed to be in the past? > > > > I don't really understand the concern. Are you afraid of rounding errors? No, what I'm concerned about is a variation of what Bryan mentions below: The wall clock time might jump at any point, but the monotonic clock won't, which might cause some of the sanity checks in this file to fail. Of course, this would only happen very occasionally, which is about the worst thing possible in terms of flakiness :) > We're actually trying to get away from using base::Time since it isn't monotonic > and can lead to significant errors if the epoch-based clock is changed between > taking two base::Times that are used to compute a duration. We should be using > TimeTicks wherever we can. Is a value from TimeTicks something that could be > used here instead? It looks like Java-based code should be able to get a > TimeTicks equivalent using > https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil.... > To provide the TimeTicks-based nav start time to clients, you can get the > TimeTicks-based nav start from the NavigationHandle in OnStart or OnCommit in > android_page_load_metrics_observer.cc. So, it looks like the time used here is actually monotonic but gives the appearance of being wall clock time (see above), so it might still be worth to expose a value with an arbitrary epoch to clients, just so that they don't assume wall clock time?
On 2016/11/14 16:17:31, Bernhard Bauer wrote: > On 2016/11/14 15:02:39, Bryan McQuade wrote: > > On 2016/11/14 at 10:39:19, droger wrote: > > > Thanks for the comments. > > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > File > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > > (right): > > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: > > long current = System.currentTimeMillis(); > > > On 2016/11/14 10:27:36, Bernhard Bauer wrote: > > > > Using a real clock here makes me queasy. In addition, do we even know that > > > > System.currentTimeMillis() uses the same underlying clock as what the > > navigation > > > > uses? I think I've traced it to base::TimeTicks::Now() > > > > > > > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > > > > which is monotonous, whereas System.currentTimeMillis() seems to return > wall > > > > clock time...? > > > > > > > > > > This time comes from android_page_load_metrics_observer.cc. See in > particular > > the call to ToJavaTime(). I believe these times are comparable. > > The thing is, AndroidPageLoadMetricsObserver gets the value from PageLoadTiming, > which (via some other hops) gets it from the NavigationParams, which get it from > TimeTicks. blink::PerformanceTiming does in fact convert from TimeTicks to > epoch-based time > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...), > so the epoch and scale is going to match, but the value is still going to be > monotonic, whereas wall clock time is not. > > > > I'm using this wall clock, since I need some kind of time that can be used > by > > another application. Note that this file is a test for a functionality exposed > > to other Android apps. > > > I would be ok to use another clock, but note that I cannot use base::Time or > > some Chromium structure. I need to use Android APIs. > > > > > > > Would it work to use a time here that is guaranteed to be in the future, > and > > > > below a time that is guaranteed to be in the past? > > > > > > I don't really understand the concern. Are you afraid of rounding errors? > > No, what I'm concerned about is a variation of what Bryan mentions below: The > wall clock time might jump at any point, but the monotonic clock won't, which > might cause some of the sanity checks in this file to fail. Of course, this > would only happen very occasionally, which is about the worst thing possible in > terms of flakiness :) > > > We're actually trying to get away from using base::Time since it isn't > monotonic > > and can lead to significant errors if the epoch-based clock is changed between > > taking two base::Times that are used to compute a duration. We should be using > > TimeTicks wherever we can. Is a value from TimeTicks something that could be > > used here instead? It looks like Java-based code should be able to get a > > TimeTicks equivalent using > > > https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil.... > > To provide the TimeTicks-based nav start time to clients, you can get the > > TimeTicks-based nav start from the NavigationHandle in OnStart or OnCommit in > > android_page_load_metrics_observer.cc. > > So, it looks like the time used here is actually monotonic but gives the > appearance of being wall clock time (see above), so it might still be worth to > expose a value with an arbitrary epoch to clients, just so that they don't > assume wall clock time? The goal of this CL is to provide the absolute time to clients. So they need to have the reference point. I'll investigate if I can convert base::TimeTicks to java's SystemClock elapsedRealtime() or uptimeMillis(), which seems to be the only thing that could work.
On 2016/11/14 16:24:59, droger wrote: > On 2016/11/14 16:17:31, Bernhard Bauer wrote: > > On 2016/11/14 15:02:39, Bryan McQuade wrote: > > > On 2016/11/14 at 10:39:19, droger wrote: > > > > Thanks for the comments. > > > > > > > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > > File > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > > > > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: > > > long current = System.currentTimeMillis(); > > > > On 2016/11/14 10:27:36, Bernhard Bauer wrote: > > > > > Using a real clock here makes me queasy. In addition, do we even know > that > > > > > System.currentTimeMillis() uses the same underlying clock as what the > > > navigation > > > > > uses? I think I've traced it to base::TimeTicks::Now() > > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > > > > > which is monotonous, whereas System.currentTimeMillis() seems to return > > wall > > > > > clock time...? > > > > > > > > > > > > > This time comes from android_page_load_metrics_observer.cc. See in > > particular > > > the call to ToJavaTime(). I believe these times are comparable. > > > > The thing is, AndroidPageLoadMetricsObserver gets the value from > PageLoadTiming, > > which (via some other hops) gets it from the NavigationParams, which get it > from > > TimeTicks. blink::PerformanceTiming does in fact convert from TimeTicks to > > epoch-based time > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...), > > so the epoch and scale is going to match, but the value is still going to be > > monotonic, whereas wall clock time is not. > > > > > > I'm using this wall clock, since I need some kind of time that can be used > > by > > > another application. Note that this file is a test for a functionality > exposed > > > to other Android apps. > > > > I would be ok to use another clock, but note that I cannot use base::Time > or > > > some Chromium structure. I need to use Android APIs. > > > > > > > > > Would it work to use a time here that is guaranteed to be in the future, > > and > > > > > below a time that is guaranteed to be in the past? > > > > > > > > I don't really understand the concern. Are you afraid of rounding errors? > > > > No, what I'm concerned about is a variation of what Bryan mentions below: The > > wall clock time might jump at any point, but the monotonic clock won't, which > > might cause some of the sanity checks in this file to fail. Of course, this > > would only happen very occasionally, which is about the worst thing possible > in > > terms of flakiness :) > > > > > We're actually trying to get away from using base::Time since it isn't > > monotonic > > > and can lead to significant errors if the epoch-based clock is changed > between > > > taking two base::Times that are used to compute a duration. We should be > using > > > TimeTicks wherever we can. Is a value from TimeTicks something that could be > > > used here instead? It looks like Java-based code should be able to get a > > > TimeTicks equivalent using > > > > > > https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil.... > > > To provide the TimeTicks-based nav start time to clients, you can get the > > > TimeTicks-based nav start from the NavigationHandle in OnStart or OnCommit > in > > > android_page_load_metrics_observer.cc. > > > > So, it looks like the time used here is actually monotonic but gives the > > appearance of being wall clock time (see above), so it might still be worth to > > expose a value with an arbitrary epoch to clients, just so that they don't > > assume wall clock time? > > The goal of this CL is to provide the absolute time to clients. So they need to > have the reference point. > > I'll investigate if I can convert base::TimeTicks to java's SystemClock > elapsedRealtime() or uptimeMillis(), which seems to be the only thing that could > work. That is exactly what the current code already does. The only thing you need to fix is the use of System.currentTimeMillis() in the test, to make sure it won't fail any sanity checks if wall clock time changes. I suggested passing a relative value to prevent clients from making false assumptions, but that could of course also be done with documentation.
On 2016/11/14 at 16:17:31, bauerb wrote: > On 2016/11/14 15:02:39, Bryan McQuade wrote: > > On 2016/11/14 at 10:39:19, droger wrote: > > > Thanks for the comments. > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > File > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > > (right): > > > > > > > > https://codereview.chromium.org/2472163003/diff/60001/chrome/android/javatest... > > > > > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:832: > > long current = System.currentTimeMillis(); > > > On 2016/11/14 10:27:36, Bernhard Bauer wrote: > > > > Using a real clock here makes me queasy. In addition, do we even know that > > > > System.currentTimeMillis() uses the same underlying clock as what the > > navigation > > > > uses? I think I've traced it to base::TimeTicks::Now() > > > > > > (https://cs.chromium.org/chromium/src/content/common/navigation_params.cc?rcl=...), > > > > which is monotonous, whereas System.currentTimeMillis() seems to return wall > > > > clock time...? > > > > > > > > > > This time comes from android_page_load_metrics_observer.cc. See in particular > > the call to ToJavaTime(). I believe these times are comparable. > > The thing is, AndroidPageLoadMetricsObserver gets the value from PageLoadTiming, which (via some other hops) gets it from the NavigationParams, which get it from TimeTicks. blink::PerformanceTiming does in fact convert from TimeTicks to epoch-based time (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...), so the epoch and scale is going to match, but the value is still going to be monotonic, whereas wall clock time is not. > > > > I'm using this wall clock, since I need some kind of time that can be used by > > another application. Note that this file is a test for a functionality exposed > > to other Android apps. > > > I would be ok to use another clock, but note that I cannot use base::Time or > > some Chromium structure. I need to use Android APIs. > > > > > > > Would it work to use a time here that is guaranteed to be in the future, and > > > > below a time that is guaranteed to be in the past? > > > > > > I don't really understand the concern. Are you afraid of rounding errors? > > No, what I'm concerned about is a variation of what Bryan mentions below: The wall clock time might jump at any point, but the monotonic clock won't, which might cause some of the sanity checks in this file to fail. Of course, this would only happen very occasionally, which is about the worst thing possible in terms of flakiness :) > > > We're actually trying to get away from using base::Time since it isn't monotonic > > and can lead to significant errors if the epoch-based clock is changed between > > taking two base::Times that are used to compute a duration. We should be using > > TimeTicks wherever we can. Is a value from TimeTicks something that could be > > used here instead? It looks like Java-based code should be able to get a > > TimeTicks equivalent using > > https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil.... > > To provide the TimeTicks-based nav start time to clients, you can get the > > TimeTicks-based nav start from the NavigationHandle in OnStart or OnCommit in > > android_page_load_metrics_observer.cc. > > So, it looks like the time used here is actually monotonic but gives the appearance of being wall clock time (see above), so it might still be worth to expose a value with an arbitrary epoch to clients, just so that they don't assume wall clock time? I don't think that helps us, unfortunately. Imagine this scenario: a. we record nav start as a pseudo wall clock time, converted from time ticks b. the system clock is changed c. a consumer of this code uses the new pseudo wall clock nav start to compute duration of time from nav start to now, by subtracting pseudo wall time nav start from System.currentTimeMillis(). The resulting delta does not reflect the actual duration that was intended to be measured - instead it's affected by the system clock change, resulting in bad data. We should never be using epoch-based time to compute durations of time, when a monotonic system clock can be used instead, due to the issue with the wall clock being changed. That's what TimeTicks and SystemClock.html#uptimeMillis() etc are for. See this comment in Chromium's time.h: https://cs.chromium.org/chromium/src/base/time/time.h?q=TimeTicks&sq=package:... "TimeTicks [is for] Tracking the amount of time a task runs." and "Note that values for Time may skew and jump around as the operating system makes adjustments to synchronize (e.g., with NTP servers)." I think we should make it easy for developers to do the safe thing, and hard but not impossible for developers to compute a time delta based on a Time value if absoulutely necessary. By providing a timestamp that's in monotonic system time rather than wall time, we encourage devs to use the right clock. If they have to compute a delta relative to a wall time, they can do a manual conversion between monotonic system uptime and epoch time, accepting the potential for risk due to clock skew, in those cases. We've been meaning to remove the base::Time navigation_start from PageLoadTiming for a while & I'm going to go ahead and do that, to prevent issues like this from emerging in the future. Can you try to use the TimeTicks from NavigationHandle, provided in the PageLoadMetricsObserver OnStart or OnCommit callbacks?
Looking at the code it is not clear to me that the TimeTicks provided by NavigationHandle is the same date. PageLoadTiming comes from blink whereas NavigationHandle comes from content. The code is kindof hard to follow though. Assuming they are the same... It seems that both TimeTicks and SystemClock.uptimeMillis() are implemented as clock_gettime(CLOCK_MONOTONIC), although they have a different precision (TimeTicks are somehow 1000 times bigger than uptimeMillis()). So I could return a TimeTicks internal value divided by 1000 and the client could compare it to times they got with SystemClock.uptimeMillis(). This seems very fragile though, since this relies on two implementation details: if either TimeTicks or SystemClock change their implementation, everything breaks. Maybe that's not that much of a problem in practice, so we can probably do it anyway? I uploaded a patch doing this (still need to update some comments). Let me also clarify the scenario I'm trying to address in this CL, since the previous discussion seemed somewhat confused. This is for a CustomTabs client, who would like to know how fast the page loaded after he sent the intent to Chrome. With this CL, he can record a start date when he sends the intent, and then receive the date of the first contentful plaint from Chrome in a CustomTabs callback. By doing the difference, he can have the actual load time. It is important to note that both the start time and the TTFCP have to be absolute, and in the same reference frame, so that it is possible to compute the difference. The client is a random Android application, not Chrome, so it does not have access to Chromium code (such as TimeTicks).
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2016/11/15 at 14:33:52, droger wrote: > Looking at the code it is not clear to me that the TimeTicks provided by NavigationHandle is the same date. > PageLoadTiming comes from blink whereas NavigationHandle comes from content. The code is kindof hard to follow though. > > Assuming they are the same... > > It seems that both TimeTicks and SystemClock.uptimeMillis() are implemented as clock_gettime(CLOCK_MONOTONIC), although they have a different precision (TimeTicks are somehow 1000 times bigger than uptimeMillis()). > So I could return a TimeTicks internal value divided by 1000 and the client could compare it to times they got with SystemClock.uptimeMillis(). > > This seems very fragile though, since this relies on two implementation details: if either TimeTicks or SystemClock change their implementation, everything breaks. > Maybe that's not that much of a problem in practice, so we can probably do it anyway? > > I uploaded a patch doing this (still need to update some comments). > > > Let me also clarify the scenario I'm trying to address in this CL, since the previous discussion seemed somewhat confused. > This is for a CustomTabs client, who would like to know how fast the page loaded after he sent the intent to Chrome. > With this CL, he can record a start date when he sends the intent, and then receive the date of the first contentful plaint from Chrome in a CustomTabs callback. > By doing the difference, he can have the actual load time. > It is important to note that both the start time and the TTFCP have to be absolute, and in the same reference frame, so that it is possible to compute the difference. > > The client is a random Android application, not Chrome, so it does not have access to Chromium code (such as TimeTicks). Ah, great, thanks for the extra information! I think it's reasonable that we assume TimeTicks and SystemClock.uptimeMillis() continue to use clock_gettime(CLOCK_MONOTONIC). We can start a thread with the TimeTicks maintainers about it to be sure. RE: PageLoadTiming and NavigationHandle having different start times, I believe they are derived from the same value but The PageLoadTiming value uses a pseudo-wall time converted from the TimeTicks. csharrison can say for sure. That said, NavigationHandle's value is definitely canonical here - we really should remove the base::Time value from PageLoadTiming. RE: computing the delta between intent start and FCP, if we provide a monotonic navigation_start, the consumer can compute this delta as (navigation_start-intent_start)+first_contentful_paint - does that sound right?
Yes the two start times are derived from the same timestamp: https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
Thanks! On 2016/11/15 16:00:02, Bryan McQuade wrote: > Ah, great, thanks for the extra information! I think it's reasonable that we > assume TimeTicks and SystemClock.uptimeMillis() continue to use > clock_gettime(CLOCK_MONOTONIC). We can start a thread with the TimeTicks > maintainers about it to be sure. > > RE: PageLoadTiming and NavigationHandle having different start times, I believe > they are derived from the same value but The PageLoadTiming value uses a > pseudo-wall time converted from the TimeTicks. csharrison can say for sure. That > said, NavigationHandle's value is definitely canonical here - we really should > remove the base::Time value from PageLoadTiming. > Great, then I think this can be the right approach. Maybe I should add a function to convert a TimeTick to a java uptime in the base::TimeTicks class itself (I notice base::Time has a ToJavaTime() function). This would avoid having this random division by 1000 in the metrics code. > RE: computing the delta between intent start and FCP, if we provide a monotonic > navigation_start, the consumer can compute this delta as > (navigation_start-intent_start)+first_contentful_paint - does that sound right? Yes, this is the plan indeed.
https://codereview.chromium.org/2472163003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2472163003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:28: * @param navigationStartMs Absolute navigation start time, in milliseconds since system boot Nit: Indent https://codereview.chromium.org/2472163003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2472163003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:45: static_cast<jlong>(navigation_start_.ToInternalValue() / 1000), From https://cs.chromium.org/chromium/src/base/time/time.h?sq=package:chromium&dr=...: "Please don't use this and do arithmetic on it, as it is more error prone than using the provided operators." Instead, you could do what Blink does: (navigation_start_ - base::TimeTicks()).InMilliseconds()
FYI: Re: TimeTicks & SystemClock.uptimeMillis() - I've ran into this before and at the time we decided not to mix the two. Let me add you guys to a thread where this was discussed. On Tue, Nov 15, 2016 at 8:12 AM, <bauerb@chromium.org> wrote: > > https://codereview.chromium.org/2472163003/diff/100001/ > chrome/android/java/src/org/chromium/chrome/browser/ > metrics/PageLoadMetrics.java > File > chrome/android/java/src/org/chromium/chrome/browser/ > metrics/PageLoadMetrics.java > (right): > > https://codereview.chromium.org/2472163003/diff/100001/ > chrome/android/java/src/org/chromium/chrome/browser/ > metrics/PageLoadMetrics.java#newcode28 > chrome/android/java/src/org/chromium/chrome/browser/ > metrics/PageLoadMetrics.java:28: > * @param navigationStartMs Absolute navigation start time, in > milliseconds since system boot > Nit: Indent > > https://codereview.chromium.org/2472163003/diff/100001/ > chrome/browser/page_load_metrics/observers/android_ > page_load_metrics_observer.cc > File > chrome/browser/page_load_metrics/observers/android_ > page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2472163003/diff/100001/ > chrome/browser/page_load_metrics/observers/android_ > page_load_metrics_observer.cc#newcode45 > chrome/browser/page_load_metrics/observers/android_ > page_load_metrics_observer.cc:45: > static_cast<jlong>(navigation_start_.ToInternalValue() / 1000), > From > https://cs.chromium.org/chromium/src/base/time/time.h? > sq=package:chromium&dr=C&rcl=1479197962&l=341: > "Please don't use this and do arithmetic on it, as it is more error > prone than using the provided operators." > > Instead, you could do what Blink does: > > (navigation_start_ - base::TimeTicks()).InMilliseconds() > > https://codereview.chromium.org/2472163003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by droger@chromium.org to run a CQ dry run
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
LGTM, just a few small questions/suggestions. https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... File base/android/time_utils.cc (right): https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... base/android/time_utils.cc:20: return RegisterNativesImpl(env); just for my own knowledge, what does this do / why is it needed? https://codereview.chromium.org/2472163003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2472163003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:28: if (!navigation_handle) this should never happen. are you encountering cases where you get a null nav handle?
https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... File base/android/time_utils.cc (right): https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... base/android/time_utils.cc:16: return TimeTicks::Now().ToInternalValue(); See my comment on https://codereview.chromium.org/2472163003/diff/100001/chrome/browser/page_lo...: Can you use `TimeTicks::Now() - TimeTicks()` (and maybe return milliseconds straight away if that is what Java is also going to use)? https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... base/android/time_utils.cc:20: return RegisterNativesImpl(env); On 2016/11/17 13:35:38, Bryan McQuade wrote: > just for my own knowledge, what does this do / why is it needed? This is a generated function in TimeUtils_jni.h that registers the address of GetTimeTicksNowUs() so it can be called from Java.
On 2016/11/17 16:14:28, Bernhard Bauer wrote: > https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... > File base/android/time_utils.cc (right): > > https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... > base/android/time_utils.cc:16: return TimeTicks::Now().ToInternalValue(); > See my comment on > https://codereview.chromium.org/2472163003/diff/100001/chrome/browser/page_lo...: > Can you use `TimeTicks::Now() - TimeTicks()` (and maybe return milliseconds > straight away if that is what Java is also going to use)? > > https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... > base/android/time_utils.cc:20: return RegisterNativesImpl(env); > On 2016/11/17 13:35:38, Bryan McQuade wrote: > > just for my own knowledge, what does this do / why is it needed? > > This is a generated function in TimeUtils_jni.h that registers the address of > GetTimeTicksNowUs() so it can be called from Java. still lgtm
droger@chromium.org changed reviewers: + torne@chromium.org
Thanks for the reviews. +torne for base/android and base/android/java/src/org/chromium/base https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... File base/android/time_utils.cc (right): https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... base/android/time_utils.cc:16: return TimeTicks::Now().ToInternalValue(); On 2016/11/17 16:14:28, Bernhard Bauer wrote: > See my comment on > https://codereview.chromium.org/2472163003/diff/100001/chrome/browser/page_lo...: > Can you use `TimeTicks::Now() - TimeTicks()` (and maybe return milliseconds > straight away if that is what Java is also going to use)? This is a code move, and this function is already used by other code. So I can't change the unit. I can do the difference with TimeTicks() though, it should not break existing code. https://codereview.chromium.org/2472163003/diff/160001/base/android/time_util... base/android/time_utils.cc:20: return RegisterNativesImpl(env); On 2016/11/17 13:35:38, Bryan McQuade wrote: > just for my own knowledge, what does this do / why is it needed? This is some black magic needed to generate the binding between Java and C++, so that GetTimeTicksNowUs() can be called from Java. https://codereview.chromium.org/2472163003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2472163003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:28: if (!navigation_handle) On 2016/11/17 13:35:38, Bryan McQuade wrote: > this should never happen. are you encountering cases where you get a null nav > handle? No, this never happened to me, I just put it there to be safe. Removed.
On 2016/11/18 10:23:21, droger wrote: > +torne for base/android and base/android/java/src/org/chromium/base Forgot to add: this is for OWNERS review, and changes there are mostly a code move.
base LGTM
base LGTM
https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... File base/android/java/src/org/chromium/base/TimeUtils.java (right): https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... base/android/java/src/org/chromium/base/TimeUtils.java:11: public class TimeUtils { Add a private empty constructor to prevent this class from being instantiated. https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:697: long offsetTick = nativeNowUs - javaNowUs; This offset will be ever so slightly different every time you run this code, right? Would it make sense to calculate the offset once lazily and use that? You could put that into TimeUtils as well. https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:32: WebContents webContents, long navigationStartMs, long firstContentfulPaintMs); Parameter name is wrong. https://codereview.chromium.org/2472163003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:43: static_cast<jlong>(navigation_start_.ToInternalValue()), Can you use `(navigation_start_ - base::TimeTicks()).ToMicroSeconds()` here as well?
https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... File base/android/java/src/org/chromium/base/TimeUtils.java (right): https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... base/android/java/src/org/chromium/base/TimeUtils.java:11: public class TimeUtils { On 2016/11/18 11:53:44, Bernhard Bauer wrote: > Add a private empty constructor to prevent this class from being instantiated. Done. https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:697: long offsetTick = nativeNowUs - javaNowUs; On 2016/11/18 11:53:44, Bernhard Bauer wrote: > This offset will be ever so slightly different every time you run this code, > right? Would it make sense to calculate the offset once lazily and use that? You > could put that into TimeUtils as well. In practice yes, and the offset will also be 0, because uptimeMillis and TimeTicks are the same thing. However, the goal of doing that is to support the case where the implementations are different. So it all depends if we assume that the offset can change over time or not. I don't really have a strong opinion on that. https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:32: WebContents webContents, long navigationStartMs, long firstContentfulPaintMs); On 2016/11/18 11:53:44, Bernhard Bauer wrote: > Parameter name is wrong. Done. https://codereview.chromium.org/2472163003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:43: static_cast<jlong>(navigation_start_.ToInternalValue()), On 2016/11/18 11:53:44, Bernhard Bauer wrote: > Can you use `(navigation_start_ - base::TimeTicks()).ToMicroSeconds()` here as > well? Done.
https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... File base/android/java/src/org/chromium/base/TimeUtils.java (right): https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... base/android/java/src/org/chromium/base/TimeUtils.java:11: public class TimeUtils { On 2016/11/18 13:43:39, droger wrote: > On 2016/11/18 11:53:44, Bernhard Bauer wrote: > > Add a private empty constructor to prevent this class from being instantiated. > > Done. Not really? https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:697: long offsetTick = nativeNowUs - javaNowUs; On 2016/11/18 13:43:39, droger wrote: > On 2016/11/18 11:53:44, Bernhard Bauer wrote: > > This offset will be ever so slightly different every time you run this code, > > right? Would it make sense to calculate the offset once lazily and use that? > You > > could put that into TimeUtils as well. > > In practice yes, and the offset will also be 0, because uptimeMillis and > TimeTicks are the same thing. Almost 0, because we call the two methods one after the other, right? Basically, this seems like it adds jitter to the returned value for no good reason, and for a client that is interested in timing, that seems less than ideal. Caching the offset value also seems to be a pattern that is used in other places when converting between different time representations; see for example blink::DocumentLoadTiming::monotonicTimeToPseudoWallTime() (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do...). > However, the goal of doing that is to support the case where the implementations > are different. > > So it all depends if we assume that the offset can change over time or not. I > don't really have a strong opinion on that.
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... File base/android/java/src/org/chromium/base/TimeUtils.java (right): https://codereview.chromium.org/2472163003/diff/180001/base/android/java/src/... base/android/java/src/org/chromium/base/TimeUtils.java:11: public class TimeUtils { On 2016/11/18 14:07:00, Bernhard Bauer wrote: > On 2016/11/18 13:43:39, droger wrote: > > On 2016/11/18 11:53:44, Bernhard Bauer wrote: > > > Add a private empty constructor to prevent this class from being > instantiated. > > > > Done. > > Not really? Sorry I messed up and this change was not in patch 9. It is in patch 10 or later though, so you should be able to see it now. https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:697: long offsetTick = nativeNowUs - javaNowUs; On 2016/11/18 14:07:00, Bernhard Bauer wrote: > On 2016/11/18 13:43:39, droger wrote: > > On 2016/11/18 11:53:44, Bernhard Bauer wrote: > > > This offset will be ever so slightly different every time you run this code, > > > right? Would it make sense to calculate the offset once lazily and use that? > > You > > > could put that into TimeUtils as well. > > > > In practice yes, and the offset will also be 0, because uptimeMillis and > > TimeTicks are the same thing. > > Almost 0, because we call the two methods one after the other, right? Right, but I don't understand the point you're trying to make. > > Basically, this seems like it adds jitter to the returned value Although please realize that we're talking about sub-millisecond noise, which is completely negligible on the scale of a page load time. It would also affect only a client that would open a large number of CustomTabs in the same run. I don't think such a client exist. > for no good reason, I gave the reason above: this is to account for uptimeMillis and TimeTicks having potentially different implementations. This is indeed arguable: maybe if one of the implementations change, they would only change the time reference, and not when they suspend the clock. If we assume this, then it is fine to cache the clock. This creates the risk of errors if they start suspending their clock differently though. If you think this reason is not good, then fine with me. > Caching the offset value also seems to be a pattern that is used in other > places Not caching the offset is also a pattern done in other places: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... Note also that in the specific example you pointed, that is a conversion between TimeTicks and wall time, which explicitly don't have a constant offset, and thus the timings will become wrong if the device goes to sleep in the middle. We don't have this problem in this CL though, as long as uptimeMillis and TimeTicks have a constant offset. Anyway, I don't think any of this matters in practice, so done.
Thanks, LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, lizeb@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2472163003/#ps260001 (title: "Cache tick offset")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1479721740349340,
"parent_rev": ["a9bbcf9d4408142b6ebe398920f952fefb06a93d", null], "commit_rev":
["9af0542466fdd8cd0dc53cc84818d6d95a312e33", null]}
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [CustomTabs] Report the navigation start as absolute time This CL adds the date of the navigation start (in milliseconds since epoch) to the page load metrics callback. This allows the client application to convert the timings into absolute times. ========== to ========== [CustomTabs] Report the navigation start as absolute time This CL adds the date of the navigation start (in milliseconds since epoch) to the page load metrics callback. This allows the client application to convert the timings into absolute times. Committed: https://crrev.com/bfa430e1a59cfac9230f836b164621beecba3f02 Cr-Commit-Position: refs/heads/master@{#433507} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bfa430e1a59cfac9230f836b164621beecba3f02 Cr-Commit-Position: refs/heads/master@{#433507}
Message was sent while issue was closed.
pasko@chromium.org changed reviewers: + pasko@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:133: private long mOffsetTick; saw this in Benoit's rebase, thoughts: 1. s/mOffsetTick/mNativeTickOffsetUs/ would be more explicit? 2. boolean members are false by default in Java. Explicit assignment here generates more code in <init> and afair is considered bad practice by Java community (however, I failed to find any reference to that in 2 minutes). https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:688: * @param navigationStartTick Absolute navigation start time, as TimeTicks. 3. I guess it needs to say something about the tick being taken from the 'native side'? https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:701: mOffsetTick = nativeNowUs - javaNowUs; aren't these two coming from different syscalls? Are we confident that the offset is going to stay the same during process lifetime?
Message was sent while issue was closed.
Thanks, will address your comments in a follow up. https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:701: mOffsetTick = nativeNowUs - javaNowUs; On 2016/11/21 17:16:37, pasko wrote: > aren't these two coming from different syscalls? Are we confident that the > offset is going to stay the same during process lifetime? In practice, they will stay the same, although this may be considered as an implementation detail and not guaranteed. There was already a debate on this, so if you're ok let's not reopen it?
Message was sent while issue was closed.
https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:701: mOffsetTick = nativeNowUs - javaNowUs; On 2016/11/21 17:28:02, droger wrote: > On 2016/11/21 17:16:37, pasko wrote: > > aren't these two coming from different syscalls? Are we confident that the > > offset is going to stay the same during process lifetime? > > In practice, they will stay the same, although this may be considered as an > implementation detail and not guaranteed. I do not quite remember the details of the discussion (perhaps I was not participating), do you have a summary documented somewhere? If it's an implementation detail, perhaps some pointer exists to show what particular part of the implementation makes sure these remain with a constant offset? I am less worried about this specific use, but more about this assumption being copied around the codebase, where the exact guarantee would not be provided. > There was already a debate on this, so if you're ok let's not reopen it? Oh, I am trying to learn the conclusion of the debate, not attempting to reopen the debate. Sorry if it looks so.
Message was sent while issue was closed.
https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2472163003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:701: mOffsetTick = nativeNowUs - javaNowUs; On 2016/11/21 at 18:56:11, pasko wrote: > On 2016/11/21 17:28:02, droger wrote: > > On 2016/11/21 17:16:37, pasko wrote: > > > aren't these two coming from different syscalls? Are we confident that the > > > offset is going to stay the same during process lifetime? > > > > In practice, they will stay the same, although this may be considered as an > > implementation detail and not guaranteed. > > I do not quite remember the details of the discussion (perhaps I was not participating), do you have a summary documented somewhere? If it's an implementation detail, perhaps some pointer exists to show what particular part of the implementation makes sure these remain with a constant offset? > > I am less worried about this specific use, but more about this assumption being copied around the codebase, where the exact guarantee would not be provided. > > > There was already a debate on this, so if you're ok let's not reopen it? > > Oh, I am trying to learn the conclusion of the debate, not attempting to reopen the debate. Sorry if it looks so. Yeah - I think the conclusion was that though both of these are currently backed by the same clock currently (clock_gettime(CLOCK_MONOTONIC)), that could potentially change and thus droger determined that we could control for that by taking the delta here, which i think is a nice approach. the remaining assumptions are that the clocks will continue to be monotonic, which seems reasonable, and to a lesser extent that they'll both continue to not tick during power saving mode, which i think is also reasonable. This was considered the best overall tradeoff in providing a high quality reference time to the CCT embedder, which doesn't have access to TimeTicks. we did not like using non-monotonic clock such as epoch wall time as reference - even though that's the most standardized clock available to consumers, it can suffer from clock skew, and we felt that providing a high quality monotonic reference time was more important here.
Message was sent while issue was closed.
On 2016/11/21 18:56:11, pasko wrote: > > On 2016/11/21 17:16:37, pasko wrote: > > > aren't these two coming from different syscalls? Are we confident that the > > > offset is going to stay the same during process lifetime? > > Oh, I am trying to learn the conclusion of the debate, not attempting to reopen > the debate. Sorry if it looks so. I think for the offset you have 3 options, which are more and more precise but require more and more assumptions: 1) recomputing the offset every time (may introduce jitter in the data) 2) compute the offset once (assumes that the offset never changes) 3) hardcode the offset to 0 (assumes that the offset never changes and is 0) I think most people were not happy with 3, and some people expressed their preference for 2). I personally prefer 1) but I consider this whole discussion to be bikeshedding at this point so I won't fight for anything.
Message was sent while issue was closed.
Follow-up CL: https://codereview.chromium.org/2520233003
Message was sent while issue was closed.
droger and bmcquade: thank you for the summary of the discussion! |
