feat(predictive-perf): add shell and base audit #2720

patrickhulce · 2017-07-21T00:36:56Z

in the name of FastMode #2703

The first commit introduces the primary structure of how the estimates will be computed.

audits/predictive-perf.js - file where a dependency graph is requested, subgraphs will be created and estimates combined
gather/computed/page-dependency-graph.js - main file responsible for creating dependency graphs and measuring their execution time, heavily leverages the logic of graph nodes found in ./dependency-graph
gather/computed/dependency-graph/* - folder where different resource/task types will be added

For easier reviewing I've split out the bulk of the logic that was here into three other PRs: #3187 #3162 #3163

patrickhulce · 2017-07-21T00:37:19Z

lighthouse-core/audits/predictive-perf.js

+    const trace = artifacts.traces[Audit.DEFAULT_PASS];
+    const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
+    return artifacts.requestPageDependencyGraph(trace, devtoolsLogs).then(graph => {
+      const rawValue = PageDependencyGraph.computeGraphDuration(graph);


will eventually be the blending of multiple graph durations

patrickhulce · 2017-07-21T00:38:00Z

lighthouse-core/gather/computed/dependency-graph/node.js

+ */
+'use strict';
+
+class Node {


other resource types will extend this base class

patrickhulce · 2017-07-21T00:38:46Z

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

+    return [];
+  }
+
+  static createGraph(traceOfTab, networkRecords) {


this method will eventually create different node types and link them in a more sophisticated way, for now it's just linking most basic initiator properties of network records

patrickhulce · 2017-07-21T00:40:20Z

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

+    return rootNode;
+  }
+
+  static computeGraphDuration(rootNode) {


this method will eventually do more browser emulation with tcp connections and main thread execution but for now it just finds the maximumal request chain and assumes a single hop

patrickhulce · 2017-07-21T00:41:48Z

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

+  }
+
+  static getNetworkInitiators(record) {
+    if (!record._initiator) return [];


I'll revisit using initiatorRequest when I flesh this out, but for now I was getting cyclic dependencies on certain runs

brendankenny

looking pretty cool.

Quick driveby: it would be great to get some jsdocs so e.g. inputs/outputs are more obvious locally :)

patrickhulce · 2017-07-21T01:07:48Z

it would be great to get some jsdocs

I made a bet with myself if I would get structural feedback or missing jsdoc comments first 😆

patrickhulce · 2017-08-30T00:28:44Z

PTAL :)

paulirish · 2017-08-30T22:07:38Z

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

+   * @param {!WebInspector.NetworkRequest} record
+   * @return {!Array<string>}
+   */
+  static getNetworkInitiators(record) {


check out https://cs.chromium.org/chromium/src/tools/android/loading/request_dependencies_lens.py?type=cs&q=f:request_dependencies_lens+_GetDependency

The logic for redirect and parser requests seem worthwhile.

The script case handles a lot of edge cases that might be worth using.

Oh this is great!! wdyt about making these changes on top of #3163 to reflect the incremental improvements?

paulirish · 2017-08-30T22:18:41Z

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

+
+  describe('#computeGraphDuration', () => {
+    it('should compute graph duration', () => {
+      //   B - C - D - E - F


I feel like it'd be really handy to get a visualization of these graphs to help debug. What did you use while developing this?

the printGraph function of PageDependencyGraph computed artifact came in crazy handy early, but mostly a lot of --inspect-brk once the cases got more complicated

Tight. that's over in #3162 . thx

brendankenny · 2017-08-31T19:33:02Z

lighthouse-core/runner.js

@@ -258,7 +258,7 @@ class Runner {
    const computedArtifacts = {};
    require('fs').readdirSync(__dirname + '/gather/computed').forEach(function(filename) {
      // Skip base class.
-      if (filename === 'computed-artifact.js') return;
+      if (filename === 'computed-artifact.js' || filename.startsWith('dependency-graph')) return;


can this be 'dependency-graph/' maybe? It took me a while to realize this was talking about the directory path rather than the start of a filename

oh ya sure that's clearer 👍

paulirish · 2017-08-31T19:30:46Z

lighthouse-core/audits/predictive-perf.js

+    return {
+      category: 'Performance',
+      name: 'predictive-perf',
+      description: 'Predicted Performance (beta)',


sorry but you need a failureDescription and non-empty helpText. :)

heh, yeah i'll cherry-pick from the future 🤖

paulirish · 2017-08-31T19:33:11Z

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

+   * @param {!WebInspector.NetworkRequest} record
+   * @return {!Array<string>}
+   */
+  static getNetworkInitiators(record) {


paulirish · 2017-08-31T20:23:32Z

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

+    }
+
+    const maxDepth = getMax(depthByNodeId.values());
+    return maxDepth * Emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.latency;


Structure-wise I feel like it'd be a tad better to have this be a generic graph computeMaxDepth method.

Optionally, I could see this latency work going into the audit rather than here in the graph computed artifact.

You are now telling me verbally that this all disappears in the next commit... so.... 💨 💨 💨 💨

brendankenny

LGTM!

brendankenny · 2017-08-31T23:16:21Z

lighthouse-core/gather/computed/dependency-graph/node.js

+   * node that was called clone is not included in the resulting filtered graph, the return will be
+   * undefined.
+   * @param {function(!Node):boolean=} predicate
+   * @return {?Node}


I think you want {!Node|undefined} (unless it can also be null)

yup you're right 👍 fixed

brendankenny · 2017-08-31T23:35:07Z

lighthouse-core/gather/computed/dependency-graph/node.js

+ */
+'use strict';
+
+class Node {


maybe add some additional description on the invariants in how it's used? Esp multiple dependencies but single root node

* feat(predictive-perf): add shell and base audit * address feedback * add invariant comments

patrickhulce commented Jul 21, 2017

View reviewed changes

brendankenny reviewed Jul 21, 2017

View reviewed changes

patrickhulce force-pushed the predictive_perf branch from a3cad60 to d29544e Compare August 1, 2017 21:29

patrickhulce changed the title ~~feat: add predictive perf shell~~ feat: add predictive perf Aug 1, 2017

patrickhulce force-pushed the predictive_perf branch 2 times, most recently from 7295761 to c34c085 Compare August 18, 2017 00:12

patrickhulce force-pushed the predictive_perf branch 2 times, most recently from 7bb2916 to 6e90985 Compare August 21, 2017 22:31

feat(predictive-perf): add shell and base audit

c448b44

patrickhulce force-pushed the predictive_perf branch 2 times, most recently from b1fde39 to 703fef1 Compare August 28, 2017 22:21

This was referenced Aug 28, 2017

feat(predictive-perf): add CPU estimation #3162

Merged

fix(predictive-perf): incremental improvements and fixes #3163

Merged

fixes for TTFB #2231

Merged

patrickhulce force-pushed the predictive_perf branch from 703fef1 to c448b44 Compare August 30, 2017 00:34

patrickhulce mentioned this pull request Aug 30, 2017

feat(predictive-perf): add network estimation #3187

Merged

patrickhulce changed the title ~~feat: add predictive perf~~ feat(predictive-perf): add shell and base audit Aug 30, 2017

patrickhulce mentioned this pull request Aug 30, 2017

fix(fast-mode): config adjustments #3189

Merged

paulirish reviewed Aug 30, 2017

View reviewed changes

brendankenny reviewed Aug 31, 2017

View reviewed changes

teddspark mentioned this pull request Aug 31, 2017

DevTools Error: Protocol error (Tracing.start): Tracing is already started #3216

Closed

paulirish requested changes Aug 31, 2017

View reviewed changes

address feedback

964cb6a

brendankenny approved these changes Sep 1, 2017

View reviewed changes

paulirish approved these changes Sep 1, 2017

View reviewed changes

add invariant comments

518fb5c

patrickhulce merged commit f473118 into master Sep 1, 2017

patrickhulce deleted the predictive_perf branch September 1, 2017 16:15

thisizkp pushed a commit to thisizkp/lighthouse that referenced this pull request Sep 2, 2017

feat(predictive-perf): add shell and base audit (GoogleChrome#2720)

396e5f8

* feat(predictive-perf): add shell and base audit * address feedback * add invariant comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(predictive-perf): add shell and base audit #2720

feat(predictive-perf): add shell and base audit #2720

patrickhulce commented Jul 21, 2017 •

edited

Loading

patrickhulce Jul 21, 2017

patrickhulce Jul 21, 2017

patrickhulce Jul 21, 2017

patrickhulce Jul 21, 2017

patrickhulce Jul 21, 2017

brendankenny left a comment

patrickhulce commented Jul 21, 2017

patrickhulce commented Aug 30, 2017

paulirish Aug 30, 2017

patrickhulce Aug 31, 2017

paulirish Aug 31, 2017

paulirish Aug 31, 2017

paulirish Aug 30, 2017

patrickhulce Aug 31, 2017

paulirish Aug 31, 2017

brendankenny Aug 31, 2017

patrickhulce Aug 31, 2017

paulirish Aug 31, 2017

patrickhulce Aug 31, 2017

paulirish Aug 31, 2017

paulirish Aug 31, 2017

brendankenny left a comment

brendankenny Aug 31, 2017

patrickhulce Sep 1, 2017

brendankenny Aug 31, 2017

patrickhulce Sep 1, 2017

feat(predictive-perf): add shell and base audit #2720

feat(predictive-perf): add shell and base audit #2720

Conversation

patrickhulce commented Jul 21, 2017 • edited Loading

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

patrickhulce commented Jul 21, 2017

patrickhulce commented Aug 30, 2017

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

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

patrickhulce commented Jul 21, 2017 •

edited

Loading