core(predictive-perf): refactor simulation logic by patrickhulce · Pull Request #3489 · GoogleChrome/lighthouse

patrickhulce · 2017-10-05T17:23:37Z

Addresses the remaining non-accuracy impacting items from code review.

Moves to use WebInspector.resourceTypes
Moves the bulk of the logic into lib/
Renames estimator -> simulator
Eliminates estimateGraph method from PageDependencyGraph artifact class
Renames the node states in Simulator
Refactors signature of simulateDownloadUntil

TODO from #3162 (comment) (Separately with analysis of impact to accuracy)

Use all trace events that have a stack trace
Improve the network initiator logic
Remove safeguards in favor of isValidDepGraph

paulirish

this works for me! only one small nit

paulirish · 2017-10-06T00:31:05Z

lighthouse-core/lib/dependency-graph/simulator/tcp-connection.js

   */
-  simulateDownloadUntil(bytesToDownload, timeAlreadyElapsed = 0, maximumTimeToElapse = Infinity) {
+  simulateDownloadUntil(bytesToDownload, options) {
+    options = Object.assign({


can we destructure AND do default values?

based on http://node.green/ i think we're good.

patrickhulce · 2017-10-07T00:09:10Z

@brendankenny did you want a whack at this too, or you good?

brendankenny

just a few style nits but otherwise LGTM

brendankenny · 2017-10-07T01:12:15Z

lighthouse-core/audits/predictive-perf.js

      Object.keys(graphs).forEach(key => {
-        const estimate = PageDependencyGraph.estimateGraph(graphs[key]);
-        const lastLongTaskEnd = PredictivePerf.getLastLongTaskEndTime(estimate.nodeTiming);
+        const results = new Simulator(graphs[key]).simulate();


I'd personally still prefer estimate for the local var here, as results is information-less :) or simulatedResults or something. results could be anything

brendankenny · 2017-10-07T01:13:02Z

lighthouse-core/audits/predictive-perf.js

 const Util = require('../report/v2/renderer/util.js');
-const PageDependencyGraph = require('../gather/computed/page-dependency-graph.js');
-const Node = require('../gather/computed/dependency-graph/node.js');
+const Simulator = require('../lib/dependency-graph/simulator/simulator.js');


I guess simulator.js's context can be figured out from its path, but maybe LoadSimulator or something for the local handle to it?

brendankenny · 2017-10-07T01:17:02Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

    this._connectionsInUse = new Set();
    this._numberInProgressByType = new Map();
+
+    this._nodes = {};


should be an array now?

wait why? I thought the whole point was to key these by their state 😆

EDIT: oh just that the keys are numbers now so you'd prefer to have array?

EDIT: oh just that the keys are numbers now so you'd prefer to have array?

yah

brendankenny · 2017-10-07T01:25:36Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

      node.record.transferSize - timingData.bytesDownloaded,
-      timingData.timeElapsed,
-      timePeriodLength - timingData.timeElapsedOvershoot
+      {


wait, why is this moving to an options object? There's only three and having them named doesn't seem better in this case...

I feel ya @brendankenny, but @paulirish was persistent

executive summary: it's easier for the caller to understand what the params are

brendankenny · 2017-10-10T17:33:19Z

rebase to pick up test fixes?

* Moves gather/computed/dependency-graph to lib/dependency-graph * Renames estimator -> simulator * Eliminates estimateGraph method from PageDependencyGraph artifact class * Renames the node states in Simulator * Refactors signature of simulateDownloadUntil

paulirish approved these changes Oct 6, 2017

View reviewed changes

brendankenny approved these changes Oct 7, 2017

View reviewed changes

patrickhulce changed the title ~~refactor(predictive-perf): remaining review items~~ refactor(predictive-perf): address remaining review items Oct 9, 2017

patrickhulce changed the title ~~refactor(predictive-perf): address remaining review items~~ core(predictive-perf): remaining review items Oct 9, 2017

patrickhulce changed the title ~~core(predictive-perf): remaining review items~~ core(predictive-perf): refactor simulation logic Oct 9, 2017

patrickhulce added 5 commits October 11, 2017 10:49

refactor(predictive-perf): use WebInspector.resourceTypes

8509da3

destructuring ftw

348655c

feedback

1916ce1

switch to nodes array

1895a8d

patrickhulce force-pushed the predictive_perf__refactor branch from fdd4f64 to 1895a8d Compare October 11, 2017 17:59

patrickhulce merged commit fc88101 into master Oct 11, 2017

patrickhulce deleted the predictive_perf__refactor branch October 11, 2017 21:55

ghost mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

Conversation

patrickhulce commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Oct 7, 2017

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patrickhulce commented Oct 5, 2017 •

edited

Loading

patrickhulce Oct 9, 2017 •

edited

Loading