core(lantern): add edges from initiatorRequest when there are duplicate records #10097

warrengm · 2019-12-11T20:52:32Z

This PR fixes some missing edges in the lantern simulation graph where a network record won't depend on its initiatorRequest if there are multiple records for the initiator. In practice I think this mainly affects cases where a script is prefetched which in turn causes multiple records and prevents connections to its initiated requests.

I fixed this by:

In network-recorder.js: make sure that initiatorRequest points to the proper script request, not the prefetch record.
In page-dependency-graph.js: fall back to the initiator request (when present) rather than the root node if the parent candidate is ambiguous.

The main changes to lantern are:

Metric                    p50 (% Error)        p90 (% Error)        p95 (% Error)       
optimisticFCP             16.9% ↔  0.0%        45.0% ↓  2.8%        54.0% ↓↓ 7.5%       
pessimisticFCP            16.4% ↔  0.2%        47.8% ↓  1.9%        73.6% ↔  0.0%       
roughEstimateOfFCP        16.2% ↔  0.0%        45.0% ↓  2.6%        61.0% ↔  0.0%       
optimisticFMP             18.1% ↔  0.0%        50.9% ↓  2.3%        62.5% ↓  4.8%       
pessimisticFMP            17.6% ↑  0.8%        52.9% ↔  0.0%        69.5% ↓  5.7%       
roughEstimateOfFMP        16.4% ↔  0.0%        49.6% ↔  0.2%        57.7% ↔  0.2%       
optimisticTTFCPUI         22.2% ↔  0.0%        62.0% ↑  4.3%        92.1% ↑  4.3%       
pessimisticTTFCPUI        20.7% ↓  0.5%        73.2% ↔  0.0%        122.4% ↓  3.6%      
roughEstimateOfTTFCPUI    22.2% ↑  0.9%        54.5% ↑  0.8%        92.1% ↑↑ 17.1%      
optimisticTTI             28.9% ↓  1.3%        56.7% ↓  1.6%        63.4% ↓  3.0%       
pessimisticTTI            20.6% ↔  0.1%        79.9% ↔  0.0%        135.3% ↔  1.3%      
roughEstimateOfTTI        23.3% ↔  0.0%        58.5% ↑  2.2%        110.3% ↑  1.4%      
optimisticSI              65.0% ↔  0.0%        83.7% ↔  0.0%        90.9% ↔  0.0%       
pessimisticSI             44.8% ↔  0.0%        84.4% ↔  0.0%        89.7% ↔  0.0%       
roughEstimateOfSI         36.0% ↑  1.1%        75.0% ↔  0.0%        82.9% ↔  0.2%       
optimisticLCP             NaN% ↓  NaN%         NaN% ↓  NaN%         NaN% ↓  NaN%        
pessimisticLCP            NaN% ↓  NaN%         NaN% ↓  NaN%         NaN% ↓  NaN%        
roughEstimateOfLCP        NaN% ↓  NaN%         NaN% ↓  NaN%         NaN% ↓  NaN%        

 ------- Fixes Summary -------
FCP on http://www.cnet.com/ - 3046 (real) 2859 (cur) 2162 (prev)
FCP on http://www.rakuten.ne.jp/ - 6988 (real) 3941 (cur) 2750 (prev)
TTFCPUI on http://www.metrolyrics.com/ - 27321 (real) 34647 (cur) 39302 (prev)
TTFCPUI on http://www.4399.com/ - 5413 (real) 4229 (cur) 3585 (prev)

 ------- Regression Summary -------
TTFCPUI on http://www.deviantart.com/ - 11309 (real) 21719 (cur) 9961 (prev)
SI on http://www.pdfqueen.com/ - 2421 (real) 3428 (cur) 2730 (prev)
TTFCPUI on http://www.pdfqueen.com/ - 3698 (real) 4727 (cur) 3803 (prev)
TTI on http://www.pdfqueen.com/ - 3698 (real) 4820 (cur) 3953 (prev)

 ------- Bucket Summary -------
Good:      242   ↓  3
OK:        185   ↑  6
Bad:       67    ↓  3

 ------- % Error Summary -------
p50:       27.4% ↑  0.3%
p90:       35.0% ↓↓ 17.9%
p95:       37.7% ↓↓ 37.3%

In pubads audits we duct taped this issue with googleads/publisher-ads-lighthouse-plugin#174.

warrengm · 2019-12-11T21:26:03Z

Thinking this a bit more, I think this can be generalized a bit further (for example, when iframes are prefetched) by making sure that lighthouse doesn't choose prefetch request as initiators (in network-recorder.js or adding links to frames in page-dependency-graph.js)

patrickhulce

thanks a ton @warrengm, great find!! 👏 👏 💯

lighthouse-core/computed/page-dependency-graph.js

patrickhulce · 2019-12-11T23:25:15Z

lighthouse-core/test/fixtures/prefetched-script.devtoolslog.json

@@ -0,0 +1,851 @@
+[
+  {
+    "method": "Page.lifecycleEvent",


would you mind running this through our ./lighthouse-core/scripts/lantern/minify-devtoolslog.js script to prune some of the unnecessary stuff?

did you get a chance to give that script a whirl?

patrickhulce · 2019-12-11T23:30:39Z

lighthouse-core/lib/network-recorder.js

+        if (!possiblePrefetchRecords.has(record.url)) {
+          possiblePrefetchRecords.set(record.url, record);
+        }
+      } else if (!typedRecords.has(record.url)) {


I wonder if we should start taking the same approach here as in page-dependency-graph and not set the initiator if it was ambiguous on the typedRecords

Possibly. I can take a look in a follow-up PR.

I did look a bit into resolving ambiguities in page-dependency-graph, and they all seemed to increase the errors in simulation. Even when I did:

const parentCandidates = (networkNodeOutput.urlToNodeMap.get(initiator) || []) .filter(candidate => candidate.startTime < node.startTime);

Though there are cases that I would expect to be resolvable. For example, sometimes gpt.js is loaded inside of the top window and inside of iframes. But, again, some of the naive filters I wrote increased the amount of error in simulation so more work would be needed to investigate. (Which is out of scope of this PR but I could look into at some point)

lighthouse-core/lib/network-recorder.js

patrickhulce · 2019-12-11T23:36:05Z

lighthouse-core/computed/page-dependency-graph.js

          // Only add the initiator relationship if the initiator is unambiguous
-          const parent = parentCandidates.length === 1 ? parentCandidates[0] : rootNode;
+          const parent = parentCandidates.length === 1 ? parentCandidates[0] : directInitiatorNode;


we probably should have been doing this all along but now that we're selecting initiators that will be later in load can we add a quick check that the request started after the parent?

patrickhulce · 2019-12-11T23:36:40Z

lighthouse-core/lib/network-recorder.js

@@ -353,7 +359,10 @@ class NetworkRecorder extends EventEmitter {
      const initiatorURL = record.initiator.url || (stackFrames[0] && stackFrames[0].url);
      // If we were redirected to this request, our initiator is that redirect, otherwise, it's the
      // initiator provided by the protocol. See https://github.com/GoogleChrome/lighthouse/pull/7352/
-      const initiator = record.redirectSource || recordsByURL.get(initiatorURL);
+      const initiator = record.redirectSource ||


should probably do that same time check about starting after the initiator here too

patrickhulce

LGTM thanks very much warren! looks awesome 😍 just a few minor nits

lighthouse-core/lib/network-recorder.js

lighthouse-core/test/computed/page-dependency-graph-test.js

lighthouse-core/lib/network-recorder.js

patrickhulce · 2019-12-13T20:22:50Z

lighthouse-core/lib/network-recorder.js

@@ -340,20 +371,26 @@ class NetworkRecorder extends EventEmitter {
    // get out the list of records & filter out invalid records
    const records = networkRecorder.getRecords().filter(record => record.isValid);

-    // create a map of all the records by URL to link up initiator
-    const recordsByURL = new Map();
+    // The initiator is unlikely to be a prefetch record which have type 'Other'


patrickhulce · 2019-12-13T20:23:28Z

lighthouse-core/test/fixtures/prefetched-script.devtoolslog.json

@@ -0,0 +1,851 @@
+[
+  {
+    "method": "Page.lifecycleEvent",


did you get a chance to give that script a whirl?

warrengm · 2020-03-04T20:37:06Z

Hi @patrickhulce, I think this is ready to merge now!

I didn't notice that my branch was behind master and then went on some travels--but I made time to finish it up today.

patrickhulce

LGTM! 🎉

patrickhulce · 2020-03-04T20:40:44Z

lighthouse-core/lib/network-recorder.js

+      }
+    }
+
+    return candidates.length ? candidates[0] : null;


friendly bump on this, if you're already thought this through a simple explanation is all I'm lookin for here :)

patrickhulce · 2020-03-10T00:46:11Z

oh btw @warrengm you've got to update https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/fixtures/lantern-master-accuracy.json now too to get the lantern data tests to pass to match the expectations, nice accuracy improvements! :)

patrickhulce · 2020-03-23T20:14:34Z

friendly ping on the two last little comments on this, would love to land before 6.0 final :)

googlebot · 2020-04-02T18:30:00Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

patrickhulce · 2020-04-02T18:31:20Z

@googlebot I consent.

googlebot · 2020-04-02T18:31:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…te records (#10097)

warrengm added 4 commits December 10, 2019 13:59

Point initiator to the script, to avoid prefetch initiators

19f617a

Guarantee edge to initiatorRequest when it is prefetched

9572faf

Add tests for initiatorRequest

688a5fc

Add tests for graph

9c05565

warrengm requested review from brendankenny and paulirish as code owners December 11, 2019 20:52

googlebot added the cla: yes label Dec 11, 2019

vercel bot deployed to staging December 11, 2019 20:52 View deployment

warrengm changed the title ~~core(lantern): Guarantee edges from initiatorRequest when there are duplicate records~~ core(lantern): Add edges from initiatorRequest when there are duplicate records Dec 11, 2019

Include ignored devtools log

0ed3422

vercel bot deployed to staging December 11, 2019 20:54 View deployment

Clean up unused var

900b031

vercel bot deployed to staging December 11, 2019 21:04 View deployment

Generalize the prefetch initiatorType check

822f5fc

vercel bot deployed to staging December 11, 2019 21:35 View deployment

paulirish assigned patrickhulce Dec 11, 2019

patrickhulce reviewed Dec 11, 2019

View reviewed changes

warrengm added 4 commits December 12, 2019 11:31

Refactor changes to network-recorder.js and add comments

abe8121

Add sanity checks for time invariants

b098bc4

Refactor network edge linking

fe25da9

Fix tests

bf3c673

vercel bot deployed to staging December 13, 2019 20:13 View deployment

patrickhulce previously approved these changes Dec 13, 2019

View reviewed changes

warrengm added 6 commits December 23, 2019 11:44

Fix typo: type -> resourceType

89ddc10

Refine initiator logic

53b6210

Clean up changes

3a9d709

Minify devtools logs

2c40e63

Add unit tests for initiatorRequest

be6afdb

Test timing for page dependency graph

0a5a255

patrickhulce assigned warrengm and unassigned patrickhulce Jan 27, 2020

Merge branch 'master' into initiator-type

a8219d3

warrengm requested a review from a team as a code owner March 4, 2020 19:57

vercel bot deployed to Preview March 4, 2020 19:57 View deployment

warrengm changed the title ~~core(lantern): Add edges from initiatorRequest when there are duplicate records~~ core(lantern): add edges from initiatorRequest when there are duplicate records Mar 4, 2020

Fix lint error (triple equals)

3813846

vercel bot deployed to Preview March 4, 2020 20:02 View deployment

patrickhulce assigned patrickhulce and unassigned warrengm Mar 4, 2020

Fix a couple type annotations

8d4c22b

vercel bot deployed to Preview March 4, 2020 20:43 View deployment

patrickhulce approved these changes Mar 4, 2020

View reviewed changes

patrickhulce added waiting4committer 6.0 labels Mar 13, 2020

patrickhulce added 2 commits April 2, 2020 13:26

Merge branch 'master' into initiator-type

6e9a1af

fix snapshots

310d0fa

vercel bot deployed to Preview April 2, 2020 18:29 View deployment

googlebot added cla: no and removed cla: yes labels Apr 2, 2020

googlebot added cla: yes and removed cla: no labels Apr 2, 2020

patrickhulce merged commit 2b61483 into GoogleChrome:master Apr 2, 2020

paulirish pushed a commit that referenced this pull request Apr 14, 2020

core(lantern): add edges from initiatorRequest when there are duplica…

0d3331d

…te records (#10097)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lantern): add edges from initiatorRequest when there are duplicate records #10097

core(lantern): add edges from initiatorRequest when there are duplicate records #10097

warrengm commented Dec 11, 2019 •

edited

Loading

warrengm commented Dec 11, 2019

patrickhulce left a comment

patrickhulce Dec 11, 2019

patrickhulce Dec 13, 2019

patrickhulce Dec 11, 2019

warrengm Dec 12, 2019

patrickhulce Dec 11, 2019

patrickhulce Dec 11, 2019

patrickhulce left a comment

patrickhulce Dec 13, 2019

patrickhulce Dec 13, 2019

warrengm commented Mar 4, 2020

patrickhulce left a comment

patrickhulce Mar 4, 2020

patrickhulce commented Mar 10, 2020

patrickhulce commented Mar 23, 2020

googlebot commented Apr 2, 2020

patrickhulce commented Apr 2, 2020

googlebot commented Apr 2, 2020

core(lantern): add edges from initiatorRequest when there are duplicate records #10097

core(lantern): add edges from initiatorRequest when there are duplicate records #10097

Conversation

warrengm commented Dec 11, 2019 • edited Loading

warrengm commented Dec 11, 2019

patrickhulce 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

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warrengm commented Mar 4, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Mar 10, 2020

patrickhulce commented Mar 23, 2020

googlebot commented Apr 2, 2020

patrickhulce commented Apr 2, 2020

googlebot commented Apr 2, 2020

warrengm commented Dec 11, 2019 •

edited

Loading