core(simulator): start nodes in observed order #5362

patrickhulce · 2018-05-25T22:11:14Z

~~I'm gonna let this one chill for a bit until I do more investigation, but basically I'm seeing graphs with some strange order and want to explore if this helps.~~

Big improvements to TTI tail error and not much change elsewhere. This should also make the smoketest savings much more reproducible/understandable.

patrickhulce · 2018-07-11T21:01:27Z

alright I rebased this against the DNS change and things are still looking positive 👍

paulirish · 2018-07-21T02:01:40Z

lighthouse-core/gather/computed/metrics/lantern-estimated-input-latency.js

@@ -94,7 +94,7 @@ class LanternEstimatedInputLatency extends LanternMetricArtifact {
      });
    }

-    return events;


I don't follow the flow perfectly but it seems like the new sorting in simulator would make this one redundant. But no?
(if not, could this one be done earlier within a more common location?)

it's redundant if the nodes all start in the order that they were queued (which used to always be true), but necessary in other cases. Even if it were true though, the fact that the .entries iterator on a map returns this in insertion order feels like an impl detail I wouldn't want important pieces of the codebase to rely on knowing since it bit me here once it changed slightly :/

I'm generally comfortable with the idea that consumers of a Map who need the results sorted going forward need to sort it themselves. As a safeguard though, I'm cool with also sorting the map before we return it based on start time. Sound good compromise?

I don't mind the double sort (which will get better when timsort in the latest V8 lands in Node), but it is going to be difficult to remember to always do this. Love this:

As a safeguard though, I'm cool with also sorting the map before we return it based on start time.

FWIW insertion-order iteration is called out very clearly for Map and Set (unlike previous iteration order issues in JS).

Maybe put a jsdoc comment on nodeTimings that insertion/iteration order is sorted based on startTime so at least the knowledge is out there somewhere?

paulirish · 2018-07-21T02:02:29Z

lighthouse-core/gather/computed/metrics/lantern-first-cpu-idle.js

@@ -56,6 +56,7 @@ class LanternFirstCPUIdle extends LanternInteractive {
      longTasks.push({start: timing.startTime, end: timing.endTime});
    }

+    longTasks.sort((a, b) => a.start - b.start);


same question. can we do the sort earlier in a more generic location?

I want to avoid the situation where someone writes a new audit and forgets they gotta sort on their own.

patrickhulce · 2018-10-22T20:34:05Z

@paulirish I rebased and added the double safeguard for insertion order 👍

patrickhulce · 2018-10-22T20:34:33Z

All lantern-impacting stats remained the same after rebasing since there have been no other lantern changes since then

brendankenny

this is looking good to me. Mostly some nits and a request for a test

brendankenny · 2018-10-22T22:43:42Z

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

+   * @param {Node[]} nodes
+   * @return {Node[]}
+   */
+  _sortNodesByStartTime(nodes) {


maybe _getNodesSortedByStartTime or something to indicate it's returning a new thing?

brendankenny · 2018-10-22T22:43:58Z

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

  _computeFinalNodeTimings() {
-    /** @type {Map<Node, LH.Gatherer.Simulation.NodeTiming>} */
-    const nodeTimings = new Map();
+    /** @type {Array<[CpuNode | NetworkNode, LH.Gatherer.Simulation.NodeTiming]>} */


CpuNode | NetworkNode -> Node? (looks like it works in the return type, at least)

brendankenny · 2018-10-22T22:49:50Z

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

@@ -419,7 +435,7 @@ class Simulator {
    // loop as long as we have nodes in the queue or currently in progress
    while (nodesReadyToStart.size || nodesInProgress.size) {
      // move all possible queued nodes to in progress
-      for (const node of nodesReadyToStart) {
+      for (const node of this._sortNodesByStartTime(Array.from(nodesReadyToStart))) {


maybe make _sortNodesByStartTime take a Set and have Array.from called internally there?

brendankenny · 2018-10-22T22:58:54Z

lighthouse-core/gather/computed/metrics/lantern-estimated-input-latency.js

@@ -94,7 +94,7 @@ class LanternEstimatedInputLatency extends LanternMetricArtifact {
      });
    }

-    return events;


I don't mind the double sort (which will get better when timsort in the latest V8 lands in Node), but it is going to be difficult to remember to always do this. Love this:

As a safeguard though, I'm cool with also sorting the map before we return it based on start time.

FWIW insertion-order iteration is called out very clearly for Map and Set (unlike previous iteration order issues in JS).

Maybe put a jsdoc comment on nodeTimings that insertion/iteration order is sorted based on startTime so at least the knowledge is out there somewhere?

brendankenny · 2018-10-22T23:00:52Z

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

    }

-    return nodeTimings;
+    // Most consumers will want the entries sorted by startTime, so insert them in that order


brendankenny · 2018-10-22T23:03:05Z

lighthouse-core/test/gather/computed/metrics/lantern-first-cpu-idle-test.js

@@ -8,6 +8,7 @@
 const LanternFirstCPUIdle = require('../../../../gather/computed/metrics/lantern-first-cpu-idle.js'); // eslint-disable-line max-len
 const assert = require('assert');

+const LanternFCPUI = require('../../../../gather/computed/metrics/lantern-first-cpu-idle');


Suggested change

const LanternFCPUI = require('../../../../gather/computed/metrics/lantern-first-cpu-idle');

const LanternFCpuI = require('../../../../gather/computed/metrics/lantern-first-cpu-idle');

never!!!! ⚔️

☠️😁😀 they need a "reject suggestion with prejudice" button

brendankenny · 2018-10-22T23:06:11Z

lighthouse-core/test/gather/computed/metrics/lantern-first-cpu-idle-test.js

+
+      assert.equal(LanternFCPUI.getFirstCPUIdleWindowStart(tasks, 0), 700);
+    });
+  });


it might be harder to set up, but maybe a Simulator test that reads in artifacts and similarly asserts that nodeTimings are in startTime order (and wouldn't have been before this change)? I think our stableSort test for a trace has caught at least one regression that was accidentally reordering traces :)

(or maybe there's an easier way to unit test it without calling private stuff; I didn't look very hard :)

brendankenny

LGTM!

patrickhulce added the chillin ❄ label May 25, 2018

patrickhulce requested review from brendankenny and paulirish as code owners May 25, 2018 22:11

patrickhulce added 4 commits June 7, 2018 11:26

core(simulator): start nodes in priority order

4fca19e

update tests

d743d78

simplify patch

d8bbb87

fixes

ff615ea

patrickhulce force-pushed the lantern_priority_order branch from 2a3375b to ff615ea Compare June 7, 2018 19:45

This was referenced Jun 12, 2018

tests(lantern): add lantern regression test scripts #5435

Merged

core: remove dependency on devtools-frontend NetworkRequest #5451

Merged

patrickhulce added 2 commits June 19, 2018 10:08

Merge branch 'master' into lantern_priority_order

3da10f8

Merge branch 'master' into lantern_priority_order

f774586

patrickhulce removed the chillin ❄ label Jul 3, 2018

patrickhulce added 4 commits July 3, 2018 15:59

update

b55d4bf

Merge branch 'master' into lantern_priority_order

0567cb3

update snapshots

0c0a392

update lantern master values

47af890

patrickhulce changed the title ~~core(simulator): start nodes in priority order~~ core(simulator): start nodes in observed order Jul 11, 2018

paulirish reviewed Jul 21, 2018

View reviewed changes

patrickhulce added 3 commits October 22, 2018 15:03

Merge branch 'master' into lantern_priority_order

aa7f3bc

add sort to simulator too

09c1a70

update proto

3acc949

patrickhulce requested a review from exterkamp as a code owner October 22, 2018 20:31

brendankenny requested changes Oct 22, 2018

View reviewed changes

feedback

8f03075

brendankenny approved these changes Oct 23, 2018

View reviewed changes

patrickhulce changed the title ~~core(simulator): start nodes in observed order~~ core(simulator): start nodes in observed order Oct 23, 2018

patrickhulce merged commit 253053f into master Oct 23, 2018

patrickhulce deleted the lantern_priority_order branch October 23, 2018 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(simulator): start nodes in observed order #5362

core(simulator): start nodes in observed order #5362

patrickhulce commented May 25, 2018 •

edited

Loading

patrickhulce commented Jul 11, 2018

paulirish Jul 21, 2018

patrickhulce Jul 23, 2018

brendankenny Oct 22, 2018

paulirish Jul 21, 2018

patrickhulce commented Oct 22, 2018

patrickhulce commented Oct 22, 2018

brendankenny left a comment

brendankenny Oct 22, 2018

patrickhulce Oct 22, 2018

brendankenny Oct 22, 2018

patrickhulce Oct 22, 2018

brendankenny Oct 22, 2018

patrickhulce Oct 22, 2018

brendankenny Oct 22, 2018

brendankenny Oct 22, 2018

brendankenny Oct 22, 2018

patrickhulce Oct 22, 2018

brendankenny Oct 22, 2018

brendankenny Oct 22, 2018

brendankenny Oct 22, 2018

patrickhulce Oct 23, 2018

brendankenny left a comment

	const LanternFCPUI = require('../../../../gather/computed/metrics/lantern-first-cpu-idle');
	const LanternFCpuI = require('../../../../gather/computed/metrics/lantern-first-cpu-idle');

core(simulator): start nodes in observed order #5362

core(simulator): start nodes in observed order #5362

Conversation

patrickhulce commented May 25, 2018 • edited Loading

patrickhulce commented Jul 11, 2018

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 Oct 22, 2018

patrickhulce commented Oct 22, 2018

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

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

patrickhulce commented May 25, 2018 •

edited

Loading