Implement Time to Interactive (TTI) for Telemetry/Profiler

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: Dominik, Assigned: jesup)

Tracking

(Depends on 2 bugs, Blocks 1 bug, {perf})

unspecified
mozilla64
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [qf:p3][perf-tools])

Attachments

(6 attachments, 10 obsolete attachments)

10.59 KB, patch
froydnj
: review+
mstange
: review+
Details | Diff | Splinter Review
2.93 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.61 KB, patch
Details | Diff | Splinter Review
19.42 KB, patch
jesup
: review+
Details | Diff | Splinter Review
4.92 KB, patch
mstange
: feedback+
Details | Diff | Splinter Review
10.46 KB, patch
mstange
: review+
Details | Diff | Splinter Review
Time to Interactive (TTI) celebrates the state when a page is able to process user interactions after loading. It requires the First Meaningful Visual to have fired.

PRD is here: https://docs.google.com/document/d/1RLqGVNL43B4oxovgR2DqmElXcBvJHJekT1bcfYooEbk/edit#
Here are the specs that currently Chromium is discussing for TTI:
https://docs.google.com/document/d/11sWqwdfd3u1TwyZhsc-fB2NcqMZ_59Kz4XKiivp1cIg/edit?pref=2&pli=1#heading=h.ir858lrmrkf5

https://github.com/tdresser/time-to-interactive

"We consider the page interactive during any time window of length > 10 seconds for which the thread executing JavaScript contains no tasks longer than 50ms. This means that, if input shows up at any time during this time window, it will be a maximum of 50ms before event handlers for this event are executed."

There are still open questions that need validation, but we'll need the probe first so that we'll be able to validate.
Here's an experiment that Chromium did on measuring TTI based on different definitions: https://docs.google.com/spreadsheets/d/1aAN2htljj8rtJBwRW3UvfEgrdUgSyFfp2Y8XpvM80IY/edit#gid=548327667

The definitions used were:
TTI-Navstart-based: the first point after navigation in which we can find a 5s windows with no long task.
TTI-FCP-based: the first point after first contentful paint in which we can find a 5s windows with no long task.
TTI-DCLEE-based: the first point after domContentLoadedEventEnd in which we can find a 5s windows with no long task.

Group thread is here: https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/progressive-web-metrics/G83qR5CR1kg/PA1hJ1k2AQAJ
Clarified the minimal scope for this bug, which is only telemetry and profiler. Developer facing numbers depend on validation and spec stabilizing.
Keywords: dev-doc-needed
Summary: Implement Time to Interactive (TTI) → Implement Time to Interactive (TTI) for Telemetry/Profiler
Related analysis on TTI from Google, comparing 4 different approaches on Chrome traces:

  https://docs.google.com/document/d/1jbvwxj_GJtiTTqFM8dsVzCIy5XeKL5qtIAvuimcXb1o/edit

In the discussion thread, tdresser did a great of job of visualizing the raw data:

https://groups.google.com/a/chromium.org/d/msg/progressive-web-metrics/Vba3oNMZEPk/IZIQfMuODQAJ
Whiteboard: [qf:p3]
Priority: -- → P3
Keywords: perf
Whiteboard: [qf:p3] → [qf:p3][perf-tools]
(Assignee)

Updated

7 months ago
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Example profile with LongTask and TTI markers
https://perfht.ml/2O1H8l8
Fix time used for TTI marker.  Note: still printf's when it adds a marker, for ease in investigating TTI stability
(Assignee)

Updated

7 months ago
Attachment #9010113 - Attachment is obsolete: true
Updated to count Idle and non-idle pauses separately
(Assignee)

Updated

7 months ago
Attachment #9010112 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #9010518 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #9013623 - Attachment is obsolete: true
yes, I realize it's TTFI currently.  If you want I can change the name.   I have no particular intenttion to expose it by default to the web; it will be behind a pref like FNBP - it's mostly for our own measurement use.
Attachment #9014191 - Flags: review?(mstange)
Attachment #9014191 - Flags: review?(bobbyholley)
(Assignee)

Updated

7 months ago
Attachment #9013625 - Attachment is obsolete: true
Attachment #9014192 - Flags: review?(bobbyholley) → review+
Comment on attachment 9014191 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9014191 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to Markus on this one.
Attachment #9014191 - Flags: review?(bobbyholley)
Comment on attachment 9014188 [details] [diff] [review]
Implement  Long Tasks API internals (not DOM access to it)

Review of attachment 9014188 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with reasonable answers to the questions and suggestions below.

::: xpcom/threads/nsThread.cpp
@@ +653,5 @@
>    , mCurrentEventStart(TimeStamp::Now())
>    , mCurrentPerformanceCounter(nullptr)
>  {
> +  mLastLongTaskEnd = mCurrentEventStart;
> +  mLastLongNonIdleTaskEnd = mCurrentEventStart;

Can we just initialize these in the initialization list, please?

@@ +675,5 @@
>    , mCurrentEventStart(TimeStamp::Now())
>    , mCurrentPerformanceCounter(nullptr)
>  {
> +  mLastLongTaskEnd = mCurrentEventStart;
> +  mLastLongNonIdleTaskEnd = mCurrentEventStart;

Same question here.

@@ +829,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsThread::GetLastLongNonIdleTaskEnd(TimeStamp* _retval) {
> +  *_retval = mLastLongNonIdleTaskEnd;

Should these getters be main-thread-only, so accesses to them are guaranteed to not be racing with event loop processing setting them?

@@ +1251,5 @@
>  
>        event->Run();
>  
> +      mozilla::TimeDuration duration;
> +      // Remember the last 50ms+ task on mainthread for Long Task.

I think if we're going to enshrine 50ms in a comment here, we should also move LONGTASK_BUSY_WINDOW_MS to this spot as a static const thing.

::: xpcom/threads/nsThread.h
@@ +155,5 @@
>  
>    static uint32_t MaxActiveThreads();
>  
> +  const mozilla::TimeStamp& LastLongTaskEnd() { return mLastLongTaskEnd; }
> +  const mozilla::TimeStamp& LastLongNonIdleTaskEnd() { return mLastLongNonIdleTaskEnd; }

These do not appear to be used in subsequent patch sets?
Attachment #9014188 - Flags: review?(nfroyd) → review+

Updated

7 months ago
Blocks: 1496544
> >    , mCurrentEventStart(TimeStamp::Now())
> >    , mCurrentPerformanceCounter(nullptr)
> >  {
> > +  mLastLongTaskEnd = mCurrentEventStart;
> > +  mLastLongNonIdleTaskEnd = mCurrentEventStart;
> 
> Can we just initialize these in the initialization list, please?

Avoiding calling ::Now() multiple times, and ensuring they're consistent.  We can't do that in the list as best I know

> > +  const mozilla::TimeStamp& LastLongTaskEnd() { return mLastLongTaskEnd; }
> > +  const mozilla::TimeStamp& LastLongNonIdleTaskEnd() { return mLastLongNonIdleTaskEnd; }
> 
> These do not appear to be used in subsequent patch sets?

The DOMNavigation changes should be using these
Comment on attachment 9014194 [details] [diff] [review]
Measure TTI (or TTFI at the moment) in Raptor TP6

Review of attachment 9014194 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/raptor/webext/raptor/measure.js
@@ +213,5 @@
> +    // TTI requires running at least 5 seconds past last "busy" point, give 100 seconds here.
> +    // Some pages will never get 5 seconds without a busy period!
> +    if (gRetryCounter <= 50) {
> +      console.log("\TTI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTI, 200);

When running locally I'm seeing it fail to get TTI when running almost every time (i.e. every tp6 page, at least once in 25 pagecycles doesn't get TTI on-time). Same behaviour consistently in production on your try push for tp6-youtube.

So the retries should be increased. When I tested before I had 1500 retries at 100ms each and the longest I saw in my limited testing to get TTI was about 1350 retries. I filed Bug 1496544 as a follow-up to not fail out raptor if TTI is never received.
Attachment #9014194 - Flags: review?(rwood)
(In reply to Randell Jesup [:jesup] from comment #17)
> > >    , mCurrentEventStart(TimeStamp::Now())
> > >    , mCurrentPerformanceCounter(nullptr)
> > >  {
> > > +  mLastLongTaskEnd = mCurrentEventStart;
> > > +  mLastLongNonIdleTaskEnd = mCurrentEventStart;
> > 
> > Can we just initialize these in the initialization list, please?
> 
> Avoiding calling ::Now() multiple times, and ensuring they're consistent. 
> We can't do that in the list as best I know

Can't we do...

  , mCurrentEventStart(TimeStamp::Now())
  [...]
  , mLastLongTaskEnd(mCurrentEventStart)
  , mLastLongNonIdleTaskEnd(mCurrentEventStart)

According to https://stackoverflow.com/questions/10720377/can-member-variables-be-used-to-initialize-other-members-in-an-initialization-li , it seems like this should work, as long as the member-variable ordering is OK (which warnings-as-errors should validate for us).
Comment on attachment 9014194 [details] [diff] [review]
Measure TTI (or TTFI at the moment) in Raptor TP6

Review of attachment 9014194 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/raptor/webext/raptor/measure.js
@@ +215,5 @@
> +    if (gRetryCounter <= 50) {
> +      console.log("\TTI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTI, 200);
> +    } else {
> +      console.log("\nunable to get a value for TTI after " + gRetryCounter + " retries\n");

If you'd like to land this now, instead of the console.log "unable to get a value'.. message here, just submit a fake result like a zero:

sendResult("tti", 0);

That would prevent the timeouts, and we can later follow up in Bug 1496544 to ignore these values when TTI is not found.
Attachment #9014843 - Flags: review?(rwood)
Attachment #9014843 - Flags: review?(mstange)
Attachment #9014843 - Flags: review?(bobbyholley)
Comment on attachment 9014843 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9014843 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the WebIDL change.
Attachment #9014843 - Flags: review?(bobbyholley) → review+
FYI, missed a spot in the latest patch, build error:

/mozilla-unified/dom/base/nsDOMNavigationTiming.cpp:332:83: error: use of undeclared identifier 'mTTI'; did you mean 'mTTFI'?"
QA Contact: svoisen
QA Contact: svoisen
Comment on attachment 9014843 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9014843 [details] [diff] [review]:
-----------------------------------------------------------------

Fixed build locally (Comment 23) and had a look. Raptor successfully captures TTFI numbers except for YouTube, it times out (see notes in-line).

R+ with the build fix and the issue re: TTFI retries time addressed.

Thanks :jesup!

::: testing/raptor/raptor/manifest.py
@@ +83,5 @@
>          if "fcp" in test_details['measure']:
>              test_settings['raptor-options']['measure']['fcp'] = True
>          if "hero" in test_details['measure']:
>              test_settings['raptor-options']['measure']['hero'] = test_details['hero'].split()
>          if "tti" in test_details['measure']:

Note: in the raptor-tp6.ini, it is still called 'tti', i.e.:

'measure = fnbpaint, hero, dcf, tti'

Which still works because of line 87 here. The INI and the line above should probably both be changed to 'ttfi' to match.

::: testing/raptor/webext/raptor/measure.js
@@ +214,4 @@
>      // Some pages will never get 5 seconds without a busy period!
> +    if (gRetryCounter <= 500) {
> +      console.log("\TTFI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTFI, 200);

An issue here - there is 100 seconds max given for TTFI data to be available. However in the raptor_tp6.ini, page_cycles is set to 30 seconds. Which means the raptor page_timeout overrides this 100 seconds after 30 seconds.

That is happening with YouTube - it seems with just 30 seconds, we can't get TTFI for Youtube at least most times. Which is causing the page_timeout to kick in at 30 seconds and the raptor test to fail, since we haven't reached the 100 second internal capturing max to substitute in a '0' value.

I recommend:

1. Not measuring TTFI on youtube at all, and

2. Reducing this internal TTFI capturing time limit to 25 seconds to avoid the page_timeout. There are 25 page cycles of 4 different pages (so far) and with a 25 second max, that can cause a very long overall tp6 job time.  Even with 25 seconds, that might cause the overall tp6 raptor taskcluster job time limit of 30 minutes to be exceeded. So this may need to be adjusted again after some exposure in production. Thanks!
Attachment #9014843 - Flags: review?(rwood) → review+
Comment on attachment 9014188 [details] [diff] [review]
Implement  Long Tasks API internals (not DOM access to it)

Review of attachment 9014188 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/ProfilerMarkerPayload.h
@@ +350,5 @@
> +class LongTaskMarkerPayload : public ProfilerMarkerPayload
> +{
> +public:
> +  LongTaskMarkerPayload(const mozilla::TimeStamp& aStartTime,
> +                   const mozilla::TimeStamp& aEndTime)

bad indent
Attachment #9014188 - Flags: review?(mstange) → review+
Comment on attachment 9014191 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9014191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +267,5 @@
> +{
> +  // Check TTI: see if it's been 5 seconds since the last Long Task
> +  TimeStamp now = TimeStamp::Now();
> +  if (mNonBlankPaint.IsNull()) {
> +    MOZ_CRASH("TTI timeout with no non-blank-paint?");

Could also use MOZ_RELEASE_ASSERT here. The return will never be hit anyway.

@@ +272,5 @@
> +    return;
> +  }
> +
> +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +  TimeStamp longtask;

longtask -> lastLongTaskEnd?

@@ +273,5 @@
> +  }
> +
> +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +  TimeStamp longtask;
> +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);

Does this differ from mLastLongNonIdleTask? If so, please add a comment about how they differ. If not, why do we need it?

@@ +276,5 @@
> +  TimeStamp longtask;
> +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);
> +  if (!longtask.IsNull()) {
> +    TimeDuration delta = now - longtask;
> +    if (delta.ToMilliseconds() < 5*1000) {

spaces around *

@@ +285,5 @@
> +      return;
> +    }
> +  }
> +  // To correctly implement TTI as proposed, we'd need to use FCP instead
> +  // of FNBP to start at, and not fire it until there are no more than 2

Please add the definitions of FCP and FNBP here, and say that we don't use FCP because we haven't implemented FCP.

@@ +294,5 @@
> +  // decreases to 2 (or record that point and let the normal timer here
> +  // handle it)
> +
> +  // TTI has occurred!  TTI is either FCP (aka for us NonBlankPaint) or
> +  // mDOMContentLoadedEventStart, whichever is later if there are no long

Weird, why DOMContentLoadedEventStart and not DOMContentLoadedEventEnd? In the time during which the DOMContentLoaded event handler runs, we're surely not interactive because we're running the handler.

@@ +310,5 @@
> +      mTTI = mNonBlankPaint;
> +    }
> +  } else {
> +    mTTI = longtask;
> +  }

This is a bit hard to read but seems correct. Maybe a helper function would make it more readable?

mTTI = MaxOrNull(longtask, MaxOrNull(mNonBlankPaint, mDOMContentLoadedEventStart);

However, this code actually doesn't match the comment. The comment says "If there was a long task, it's the long task time. Otherwise it's either FCP or DCLES." The code says "it's whatever completed last, out of { long task, FCP, DCLES }."
If the code is correct, please adjust the comment.

@@ +338,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mLastLongNonIdleTask = aWhen;
> +  if (mTTITimer) {
> +    // move check time to 5 seconds from now.  Note that this may *never* end up firing

Maybe say "Reset the check time" to make it clearer that the existing timer is canceled?

@@ +340,5 @@
> +  mLastLongNonIdleTask = aWhen;
> +  if (mTTITimer) {
> +    // move check time to 5 seconds from now.  Note that this may *never* end up firing
> +    // on a site that has periodic long tasks or too many active loads!
> +    mTTITimer->InitWithNamedFuncCallback(TTITimeoutCallback, this, 5*1000,

spaces around * please

@@ +372,5 @@
>      profiler_add_marker(marker.get());
>    }
>  #endif
>  
> +  // start window for TTI

Please remove this comment or make it a full sentence. But the comment 6 lines further down already seems sufficient.

::: dom/base/nsDOMNavigationTiming.h
@@ +96,5 @@
>    DOMTimeMilliSec GetTimeToNonBlankPaint() const
>    {
>      return TimeStampToDOM(mNonBlankPaint);
>    }
> +  DOMTimeMilliSec GetTimeToTTI() const

"time to time to interactive" sounds a tad repetitive, but oh well

@@ +175,5 @@
>  
> +  static void TTITimeoutCallback(nsITimer* aTimer, void *aClosure);
> +  void TTITimeout(nsITimer* aTimer);
> +
> +  void NotifyLongTask(mozilla::TimeStamp aWhen);

Is this the time when the long task started or when it ended? Please add a comment.
Attachment #9014191 - Flags: review?(mstange)
Attachment #9014843 - Flags: review?(mstange) → review+
> > +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> > +  TimeStamp longtask;
> > +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);
> 
> Does this differ from mLastLongNonIdleTask? If so, please add a comment
> about how they differ. If not, why do we need it?

Turns out we don't need it (at least for now, though we might for exposing LongTasks behind a pref) and nothing was calling NotifyLongtask().  Removed for now
 
> > +  // TTI has occurred!  TTI is either FCP (aka for us NonBlankPaint) or
> > +  // mDOMContentLoadedEventStart, whichever is later if there are no long
> 
> Weird, why DOMContentLoadedEventStart and not DOMContentLoadedEventEnd? In
> the time during which the DOMContentLoaded event handler runs, we're surely
> not interactive because we're running the handler.

yes - though the "time" of an event would be when it fires, not when user code from that firing finishes.  I used to have it as End, and reading a different doc on this from google they do say End there, so switching it.

> However, this code actually doesn't match the comment. The comment says "If
> there was a long task, it's the long task time. Otherwise it's either FCP or
> DCLES." The code says "it's whatever completed last, out of { long task,
> FCP, DCLES }."
> If the code is correct, please adjust the comment.

I adjusted the comment; the "spec" is less than clear here.
Attachment #9016027 - Flags: feedback?(mstange)
(Assignee)

Updated

7 months ago
Attachment #9014843 - Attachment is obsolete: true
Comment on attachment 9016030 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9016030 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+ from bholley, rwood and mstange (just rebased on top of changes to the main TTI/TTFI patch)
Attachment #9016030 - Flags: review+
updated patch
Attachment #9016031 - Flags: review?(mstange)
(Assignee)

Updated

7 months ago
Attachment #9014191 - Attachment is obsolete: true
Comment on attachment 9016027 [details] [diff] [review]
interdiffs for review response f=mstange

Review of attachment 9016027 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +264,5 @@
>  
> +// Return the max of t1 and t2, or max N ms after the earlier one.
> +// Handle the edge case of a late wakeup where there was >5 seconds after
> +// DOMContentLoaded or lastLongTaskEnded without the other, but it happened
> +// after 5 seconds and before we woke up.

I don't understand this sentence. Can you rephrase it in terms of aT1, aT2 and aWindowSize?

@@ +266,5 @@
> +// Handle the edge case of a late wakeup where there was >5 seconds after
> +// DOMContentLoaded or lastLongTaskEnded without the other, but it happened
> +// after 5 seconds and before we woke up.
> +static TimeStamp&
> +MaxWithWindow(TimeStamp& aT1, TimeStamp& aT2, uint32_t aWindowSize)

Please use const references, and make aWindowSize a TimeDuration so that you don't have to add a comment about what unit it is in.
Posted patch review updateSplinter Review
Attachment #9016057 - Flags: feedback?(mstange)
Approximates TimeToIinteractive or really TimeToFirstInteractive
* * *
Bug 1299118: review update
Attachment #9016060 - Flags: review?(mstange)
(Assignee)

Updated

7 months ago
Attachment #9016031 - Attachment is obsolete: true
Attachment #9016031 - Flags: review?(mstange)
Comment on attachment 9016057 [details] [diff] [review]
review update

Review of attachment 9016057 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +266,5 @@
> +// Handles the edge case of a late wakeup: where there was more than Nms
> +// after one (of aT1 or aT2) without the other, but the other happened
> +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> +// if the window is 5 seconds.

Sorry I'm still having trouble understanding the "waking up" metaphor. Which time here corresponds to the time at which we woke up?
(Assignee)

Updated

7 months ago
Attachment #9016027 - Attachment is obsolete: true
Attachment #9016027 - Flags: feedback?(mstange)
> > +// Handles the edge case of a late wakeup: where there was more than Nms
> > +// after one (of aT1 or aT2) without the other, but the other happened
> > +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> > +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> > +// if the window is 5 seconds.
> 
> Sorry I'm still having trouble understanding the "waking up" metaphor. Which
> time here corresponds to the time at which we woke up?

We start a 5s timer at a longtask at X.
At X+5 we should wake up, and if there's been no DCLEnd, we would put the TTFI at X.
If we instead wake up late (say X+10), and DCLEnd happend at X+7, we would look at that and say "ok, TTFI should be at X+7" if we don't check an explicit window (of 5s from X).

So, no time (aT1 or aT2) is when we woke up, just that waking up late might allow the second event to have occurred -- outside of the 5s window -- and we might mis-calculate.  Admittedly unlikely and non-critical, but easy to fix like this.
Approximates TimeToIinteractive or really TimeToFirstInteractive
* * *
Bug 1299118: review update
Attachment #9016292 - Flags: review?(mstange)
(Assignee)

Updated

7 months ago
Attachment #9016060 - Attachment is obsolete: true
Attachment #9016060 - Flags: review?(mstange)
Comment on attachment 9016292 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9016292 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! It's clear to me now. Just three tiny nitpicks left.

::: dom/base/nsDOMNavigationTiming.cpp
@@ +265,5 @@
> +// Return the max of aT1 and aT2, or the lower of the two if there's more
> +// than Nms (the window size) between them.  In other words, the window
> +// starts at the lower of aT1 and aT2, and we only want to respect
> +// timestamps within the window (and pick the max of those).
> +

Sorry, really nitpicking here now, but could you make this empty line also start with // so that it's clear that the two paragraphs belong to the same comment?

@@ +266,5 @@
> +// than Nms (the window size) between them.  In other words, the window
> +// starts at the lower of aT1 and aT2, and we only want to respect
> +// timestamps within the window (and pick the max of those).
> +
> +// Handles the edge case of a late wakeup: where there was more than Nms

Could you change "Handles" to "This approach handles", so that it's clearer that the explanation that follows only describes implications of the sentence above, and doesn't add any further requirements?

@@ +272,5 @@
> +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> +// if the window is 5 seconds.
> +static const TimeStamp&
> +MaxWithWindow(const TimeStamp& aT1, const TimeStamp& aT2,

How would you feel about the name MaxWithinWindowBeginningAtMin, just to drive the point home?
Attachment #9016292 - Flags: review?(mstange) → review+
Attachment #9016057 - Flags: feedback?(mstange) → feedback+

Comment 39

7 months ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7bb583fbb5
Implement  Long Tasks API internals (not DOM access to it) r=froyd,mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5adc30bdf7f
non-spec TimeToFirstInteractive implementation behind a pref r=mstange,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/14451eb9a2b8
Measure TTI (or TTFI at the moment) in Raptor TP6 r=rwood

Comment 41

7 months ago
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a40850883ff
Backed out 3 changesets for ES lint failures

Comment 42

7 months ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f83ebaabaa3
Implement  Long Tasks API internals (not DOM access to it) r=froyd,mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3cf6dbf466
non-spec TimeToFirstInteractive implementation behind a pref r=mstange,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/69dd27f16b71
Measure TTI (or TTFI at the moment) in Raptor TP6 r=rwood

Comment 43

7 months ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd60c9679d21
Android bustage fix for no-gecko-profiler builds r=bustage
Depends on: 1498610
(Assignee)

Updated

6 months ago
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.