core(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

patrickhulce · 2020-08-11T23:04:29Z

Summary
Another noop tracehouse change that ~~will make it easier for fraggle rock and tracehouse consumers in the future~~ is needed for FR work now. This PR splits apart the computeTraceOfTab method into a few components.

computeTimeOrigin, a way to choose how you want your time origin to be computed
computeKeyTimingsForFrame, the bulk of the previous trace of tab logic given a specific frame and time origin
computeTraceEnd, helper function to prevent that bug from sneaking into the two places we use it :)

It has one user-facing impact which is a minor bug fix to our traceEnd computation. Previously we did not consider the maximum ts + dur time so it changes by a few ms in some situations.

Related Issues/PRs
ref #8984 #10325 #9519

connorjclark · 2020-08-11T23:06:21Z

fraggle rock and tracehouse

maybe we should start a glossary :)

tracehouse
fraggle rock
smokehouse
lightrider
gatherer
audit
lantern

etc...

connorjclark

i only have nits

lighthouse-core/lib/tracehouse/trace-processor.js

Co-authored-by: Connor Clark <[email protected]>

patrickhulce · 2020-10-12T16:12:30Z

🦗

patrickhulce · 2020-10-19T14:30:53Z

@adamraine would you mind taking a look at this? :)

lighthouse-core/lib/tracehouse/trace-processor.js

brendankenny

😬 😳

is needed for FR work now

How is firstResourceSendRequest going to be used in FR? It seems like it would have a number of possible issues for anything but a 'navigation'

lighthouse-core/lib/tracehouse/trace-processor.js

patrickhulce · 2020-10-19T21:00:30Z

How is firstResourceSendRequest going to be used in FR?

It's not. This refactor to eliminate automatic selection of time origin based on navigation is required, i.e. "split timeOrigin determination out of computeTraceOfTab" ;)

Co-authored-by: Brendan Kenny <[email protected]>

lighthouse-core/lib/tracehouse/trace-processor.js

adamraine

LGTM

brendankenny

some last questions but regardless of the answers this LGTM2 and is ready to land

lighthouse-core/lib/tracehouse/trace-processor.js

brendankenny · 2020-10-21T19:30:54Z

lighthouse-core/lib/tracehouse/trace-processor.js

+      });
+
+    // Ensure our traceEnd reflects all page activity.
+    const traceEnd = this.computeTraceEnd(trace.traceEvents, timeOriginEvt);


any reason not to just pass trace.traceEvents into computeKeyTimingsForFrame and do traceEnd for real in there?

As in pass both frameEvents and all trace events?

Primary reason I did not do that is it violates the veil of abstraction that computeKeyTimingsForFrame well, computes the key timings for a frame :)

If the double computation is problematic I'd prefer to modify the return type of computeKeyTimingsForFrame to omit traceEnd entirely as a proper fix. The duplication of all the types to avoid a double computation didn't seem to be worth the tradeoff in confusion initially since the trace end computation is cheap (~0.5ms/MB of trace). Prefer this approach?

If the double computation is problematic

It's not fatally problematic, no more so than breaking the veil :) And definitely not worried about the computation cost. I was just curious about your thinking on which was less confusing, passing in both sets of events or fake computing it and then doing it for real. Breaking the type down might be worse but it's possible it would be ok...e.g. with an inferred return type on computeKeyTimingsForFrame, maybe?

I'm also trying to be more literal in my review comments. I was 95% sure you had already considered this question so I was 95% really just asking, and 5% suggesting considering it and then just asking as a consideration follow up :)

Breaking the type down might be worse but it's possible it would be ok...e.g. with an inferred return type on computeKeyTimingsForFrame, maybe?

I like it SG!

If the double computation is problematic I'd prefer to modify the return type of computeKeyTimingsForFrame to omit traceEnd entirely as a proper fix.

Just noticed FrameTimings is a new typedef, and yeah, this would be the proper fix and it would be great if it's a simple modification to the Omit, but I don't know if there's an easy way to do that one level deep, so don't worry about it if there's not.

brendankenny · 2020-10-21T19:34:02Z

lighthouse-core/computed/trace-of-tab.js

@@ -19,6 +19,15 @@ class LHTraceProcessor extends TraceProcessor {
    return new LHError(LHError.errors.NO_NAVSTART);
  }

+  /**
+   * This isn't ever actually used, but might one day be.


lol if this is true (and there aren't concrete FR plans for it), why are we refactoring and adding 'firstResourceSendRequest'? Why not refactor but have 'lastNavigationStart' be the only option right now?

well, looking at the excellent comment on computeTimeOrigin, maybe the simpler thing is to change this to something more concrete like

Suggested change

* This isn't ever actually used, but might one day be.

* This isn't currently used, but will be when the time origin of trace processing is changed.

* @see {TraceProcessor.computeTimeOrigin}

(the @see doesn't actually work like for that yet, but will soon(!), though if the thing isn't imported, I don't know if you actually have to do something like @see {import('../../path/to/trace-processor.js').computeTimeOrigin} or what)

why are we [...] adding 'firstResourceSendRequest'?

firstResourceSendRequest won't be used for FR, but will be used for the #8984 line of work that will continue amidst FR.

Why not refactor but have 'lastNavigationStart' be the only option right now?

That's another option but a motivating example to validate the split seemed worthwhile to pair with the refactor given its simplicity and test uninterestingness if there's only one option :)

maybe the simpler thing is to change this to something more concrete like

sg 👍

brendankenny

ok, two real things, sorry :)

brendankenny · 2020-10-21T19:49:09Z

lighthouse-core/lib/lh-error.js

@@ -242,6 +242,11 @@ const ERRORS = {
    message: UIStrings.badTraceRecording,
    lhrRuntimeError: true,
  },
+  NO_RESOURCE_SEND: {


NO_FIRST_RESOURCE? NO_FIRST_RESOURCE_SEND?

Does this need to be added to the proto, too, since it's a lhrRuntimeError? Again it's a bit premature, but we might easily forget to do it when the error is live.

Does this need to be added to the proto, too, since it's a lhrRuntimeError? Again it's a bit premature, but we might easily forget to do it when the error is live.

Yeah sg might as well do it now too.

re: NO_FIRST_RESOURCE_SEND, any particular reason you'd like to see the addition of FIRST? It seemed like unnecessarily specific information to add to me that might lead to narrower usage later. The only way there can be no first resource request is if there were none at all :)

re: NO_FIRST_RESOURCE_SEND, any particular reason you'd like to see the addition of FIRST? It seemed like unnecessarily specific information to add to me that might lead to narrower usage later. The only way there can be no first resource request is if there were none at all :)

it's just short and I was wondering if there's more context we can give it. That's a very good point, though :)

"Resource send" is kind of confusing as representing the request for a resource, though (ResourceSendRequest does make sense with the other trace event names next to it). Maybe NO_RESOURCE_REQUEST? A slight step away from the trace event name but slightly more understandable on its own.

I'll stop bikeshedding after this :)

NO_RESOURCE_REQUEST

sold :)

lighthouse-core/test/lib/tracehouse/trace-processor-test.js

brendankenny

still LGTM :)

brendankenny · 2020-10-22T17:01:29Z

lighthouse-core/lib/tracehouse/trace-processor.js

+      });
+
+    // Ensure our traceEnd reflects all page activity.
+    const traceEnd = this.computeTraceEnd(trace.traceEvents, timeOriginEvt);


If the double computation is problematic I'd prefer to modify the return type of computeKeyTimingsForFrame to omit traceEnd entirely as a proper fix.

Just noticed FrameTimings is a new typedef, and yeah, this would be the proper fix and it would be great if it's a simple modification to the Omit, but I don't know if there's an easy way to do that one level deep, so don't worry about it if there's not.

core(tracehouse): split timeOrigin determination out from traceOfTab

74d3373

patrickhulce requested a review from a team as a code owner August 11, 2020 23:04

patrickhulce requested review from brendankenny and removed request for a team August 11, 2020 23:04

googlebot added the cla: yes label Aug 11, 2020

devtools-bot assigned brendankenny Aug 11, 2020

devtools-bot added the waiting4reviewer label Aug 11, 2020

connorjclark reviewed Aug 11, 2020

View reviewed changes

lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved

lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved

Apply suggestions from code review

f8eaf86

Co-authored-by: Connor Clark <[email protected]>

vercel bot deployed to Preview August 11, 2020 23:28 View deployment

patrickhulce mentioned this pull request Aug 25, 2020

Flow Support (Fraggle Rock) #11313

Closed

patrickhulce mentioned this pull request Sep 29, 2020

RFC: Lighthouse Subpackage Proposal (report UI and tracehouse) #9519

Closed

patrickhulce assigned adamraine and unassigned brendankenny Oct 19, 2020

adamraine reviewed Oct 19, 2020

View reviewed changes

lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved

adamraine added waiting4committer and removed waiting4reviewer labels Oct 19, 2020

brendankenny reviewed Oct 19, 2020

View reviewed changes

Update lighthouse-core/lib/tracehouse/trace-processor.js

13674bf

Co-authored-by: Brendan Kenny <[email protected]>

vercel bot deployed to Preview October 20, 2020 20:15 View deployment

feedback

202fbff

patrickhulce added waiting4reviewer and removed waiting4committer labels Oct 20, 2020

vercel bot deployed to Preview October 20, 2020 20:33 View deployment

adamraine reviewed Oct 20, 2020

View reviewed changes

lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved

save

555839f

vercel bot deployed to Preview October 20, 2020 21:16 View deployment

adamraine approved these changes Oct 20, 2020

View reviewed changes

brendankenny approved these changes Oct 21, 2020

View reviewed changes

brendankenny reviewed Oct 21, 2020

View reviewed changes

brendankenny added waiting4committer and removed waiting4reviewer labels Oct 21, 2020

feedback

9004793

patrickhulce added waiting4reviewer and removed waiting4committer labels Oct 22, 2020

vercel bot deployed to Preview October 22, 2020 03:31 View deployment

patrickhulce assigned brendankenny and unassigned adamraine Oct 22, 2020

brendankenny approved these changes Oct 22, 2020

View reviewed changes

patrickhulce added 2 commits October 22, 2020 12:03

infer frame timings type

158b901

Merge branch 'master' into time_origin_method

face33b

vercel bot deployed to Preview October 22, 2020 17:05 View deployment

NO_RESOURCE_REQUEST

ccb3f29

vercel bot deployed to Preview October 22, 2020 17:07 View deployment

patrickhulce merged commit 55995ff into master Oct 22, 2020

patrickhulce deleted the time_origin_method branch October 22, 2020 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

core(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

patrickhulce commented Aug 11, 2020 •

edited

Loading

connorjclark commented Aug 11, 2020 •

edited

Loading

connorjclark left a comment

patrickhulce commented Oct 12, 2020

patrickhulce commented Oct 19, 2020

brendankenny left a comment •

edited

Loading

patrickhulce commented Oct 19, 2020

adamraine left a comment

brendankenny left a comment

brendankenny Oct 21, 2020

patrickhulce Oct 22, 2020

brendankenny Oct 22, 2020

patrickhulce Oct 22, 2020

brendankenny Oct 22, 2020

brendankenny Oct 21, 2020

brendankenny Oct 21, 2020

patrickhulce Oct 22, 2020

brendankenny left a comment

brendankenny Oct 21, 2020

brendankenny Oct 21, 2020

patrickhulce Oct 22, 2020

brendankenny Oct 22, 2020

patrickhulce Oct 22, 2020

brendankenny left a comment

brendankenny Oct 22, 2020

	* This isn't ever actually used, but might one day be.
	* This isn't currently used, but will be when the time origin of trace processing is changed.
	* @see {TraceProcessor.computeTimeOrigin}

core(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

core(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

Conversation

patrickhulce commented Aug 11, 2020 • edited Loading

connorjclark commented Aug 11, 2020 • edited Loading

connorjclark left a comment

Choose a reason for hiding this comment

patrickhulce commented Oct 12, 2020

patrickhulce commented Oct 19, 2020

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Oct 19, 2020

adamraine left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Aug 11, 2020 •

edited

Loading

connorjclark commented Aug 11, 2020 •

edited

Loading

brendankenny left a comment •

edited

Loading