Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 14 commits into from
Oct 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4fca19e
core(simulator): start nodes in priority order
patrickhulce May 25, 2018
d743d78
update tests
patrickhulce May 25, 2018
d8bbb87
simplify patch
patrickhulce Jun 7, 2018
ff615ea
fixes
patrickhulce Jun 7, 2018
3da10f8
Merge branch 'master' into lantern_priority_order
patrickhulce Jun 19, 2018
f774586
Merge branch 'master' into lantern_priority_order
patrickhulce Jul 3, 2018
b55d4bf
update
patrickhulce Jul 3, 2018
0567cb3
Merge branch 'master' into lantern_priority_order
patrickhulce Jul 11, 2018
0c0a392
update snapshots
patrickhulce Jul 11, 2018
47af890
update lantern master values
patrickhulce Jul 11, 2018
aa7f3bc
Merge branch 'master' into lantern_priority_order
patrickhulce Oct 22, 2018
09c1a70
add sort to simulator too
patrickhulce Oct 22, 2018
3acc949
update proto
patrickhulce Oct 22, 2018
8f03075
feedback
patrickhulce Oct 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-core/gather/computed/metrics/lantern-estimated-input-latency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class LanternEstimatedInputLatency extends LanternMetric {
});
}

return events;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return events.sort((a, b) => a.start - b.start);
}
}

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/gather/computed/metrics/lantern-first-cpu-idle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class LanternFirstCPUIdle extends LanternInteractive {
longTasks.push({start: timing.startTime, end: timing.endTime});
}

longTasks.sort((a, b) => a.start - b.start);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Require here to resolve circular dependency.
const FirstCPUIdle = require('./first-cpu-idle');
return FirstCPUIdle.findQuietWindow(fmpTimeInMs, Infinity, longTasks);
Expand Down
28 changes: 22 additions & 6 deletions lighthouse-core/lib/dependency-graph/simulator/simulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,17 @@ class Simulator {
});
}

/**
* @param {Set<Node>} nodes
* @return {Node[]}
*/
_getNodesSortedByStartTime(nodes) {
return Array.from(nodes).sort((nodeA, nodeB) => {
// Sort nodes by startTime to match original execution order
return nodeA.startTime - nodeB.startTime;
});
}

/**
* @param {Node} node
* @param {number} totalElapsedTime
Expand Down Expand Up @@ -354,18 +365,23 @@ class Simulator {
}
}

/**
* @return {Map<Node, LH.Gatherer.Simulation.NodeTiming>}
*/
_computeFinalNodeTimings() {
/** @type {Map<Node, LH.Gatherer.Simulation.NodeTiming>} */
const nodeTimings = new Map();
/** @type {Array<[Node, LH.Gatherer.Simulation.NodeTiming]>} */
const nodeTimingEntries = [];
for (const [node, timing] of this._nodeTimings) {
nodeTimings.set(node, {
nodeTimingEntries.push([node, {
startTime: timing.startTime,
endTime: timing.endTime,
duration: timing.endTime - timing.startTime,
});
}]);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

nodeTimingEntries.sort((a, b) => a[1].startTime - b[1].startTime);
return new Map(nodeTimingEntries);
}

/**
Expand Down Expand Up @@ -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._getNodesSortedByStartTime(nodesReadyToStart)) {
this._startNodeIfPossible(node, totalElapsedTime);
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/audits/__snapshots__/metrics-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`Performance: metrics evaluates valid input correctly 1`] = `
Object {
"estimatedInputLatency": 104,
"estimatedInputLatency": 100,
"estimatedInputLatencyTs": undefined,
"firstCPUIdle": 3351,
"firstCPUIdleTs": undefined,
Expand Down Expand Up @@ -32,7 +32,7 @@ Object {
"observedSpeedIndexTs": 225414776724,
"observedTraceEnd": 12540,
"observedTraceEndTs": 225426711887,
"speedIndex": 1656,
"speedIndex": 1657,
"speedIndexTs": undefined,
}
`;
8 changes: 4 additions & 4 deletions lighthouse-core/test/audits/__snapshots__/predictive-perf-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`Performance: predictive performance audit should compute the predicted values 1`] = `
Object {
"optimisticEIL": 101,
"optimisticEIL": 93,
"optimisticFCP": 911,
"optimisticFMP": 1211,
"optimisticSI": 605,
Expand All @@ -11,13 +11,13 @@ Object {
"pessimisticEIL": 158,
"pessimisticFCP": 911,
"pessimisticFMP": 1498,
"pessimisticSI": 1630,
"pessimisticSI": 1631,
"pessimisticTTFCPUI": 3502,
"pessimisticTTI": 3502,
"roughEstimateOfEIL": 104,
"roughEstimateOfEIL": 100,
"roughEstimateOfFCP": 911,
"roughEstimateOfFMP": 1355,
"roughEstimateOfSI": 1656,
"roughEstimateOfSI": 1657,
"roughEstimateOfTTFCPUI": 3351,
"roughEstimateOfTTI": 3427,
}
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/audits/byte-efficiency/__snapshots__/byte-efficiency-audit-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
exports[`Byte efficiency base audit should allow overriding of computeWasteWithTTIGraph 1`] = `
Object {
"default": 1330,
"justTTI": 950,
"justTTI": 800,
}
`;

exports[`Byte efficiency base audit should create load simulator with the specified settings 1`] = `1330`;

exports[`Byte efficiency base audit should create load simulator with the specified settings 2`] = `22950`;
exports[`Byte efficiency base audit should create load simulator with the specified settings 2`] = `22730`;
156 changes: 78 additions & 78 deletions lighthouse-core/test/fixtures/lantern-master-computed-values.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions ...ouse-core/test/gather/computed/metrics/__snapshots__/estimated-input-latency-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

exports[`Metrics: EIL should compute a simulated value 1`] = `
Object {
"optimistic": 101,
"optimistic": 93,
"pessimistic": 158,
"timing": 104,
"timing": 100,
}
`;
4 changes: 2 additions & 2 deletions ...e/test/gather/computed/metrics/__snapshots__/lantern-estimated-input-latency-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

exports[`Metrics: Lantern EIL should compute a simulated value 1`] = `
Object {
"optimistic": 101,
"optimistic": 93,
"pessimistic": 158,
"timing": 104,
"timing": 100,
}
`;
4 changes: 2 additions & 2 deletions lighthouse-core/test/gather/computed/metrics/__snapshots__/lantern-speed-index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Metrics: Lantern Speed Index should compute predicted value 1`] = `
Object {
"optimistic": 605,
"pessimistic": 1630,
"timing": 1656,
"pessimistic": 1631,
"timing": 1657,
}
`;
4 changes: 2 additions & 2 deletions lighthouse-core/test/gather/computed/metrics/__snapshots__/speed-index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Metrics: Speed Index should compute a simulated value 1`] = `
Object {
"optimistic": 605,
"pessimistic": 1630,
"timing": 1656,
"pessimistic": 1631,
"timing": 1657,
}
`;
16 changes: 16 additions & 0 deletions lighthouse-core/test/gather/computed/metrics/lantern-first-cpu-idle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,20 @@ describe('Metrics: Lantern TTFCPUI', () => {
assert.ok(result.optimisticGraph, 'should have created optimistic graph');
assert.ok(result.pessimisticGraph, 'should have created pessimistic graph');
});

describe('#getFirstCPUIdleWindowStart', () => {
it('should sort tasks', () => {
const tasks = new Map([
[{type: 'cpu'}, {startTime: 600, endTime: 700, duration: 100}],
[{type: 'cpu'}, {startTime: 300, endTime: 400, duration: 100}],
[{type: 'cpu'}, {startTime: 0, endTime: 100, duration: 100}],
[{type: 'cpu'}, {startTime: 100, endTime: 200, duration: 100}],
[{type: 'cpu'}, {startTime: 500, endTime: 600, duration: 100}],
[{type: 'cpu'}, {startTime: 200, endTime: 300, duration: 100}],
[{type: 'cpu'}, {startTime: 400, endTime: 500, duration: 100}],
]);

assert.equal(LanternFirstCPUIdle.getFirstCPUIdleWindowStart(tasks, 0), 700);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

});
30 changes: 29 additions & 1 deletion lighthouse-core/test/lib/dependency-graph/simulator/simulator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const NetworkNode = require('../../../../lib/dependency-graph/network-node');
const CpuNode = require('../../../../lib/dependency-graph/cpu-node');
const Simulator = require('../../../../lib/dependency-graph/simulator/simulator');
const DNSCache = require('../../../../lib/dependency-graph/simulator/dns-cache');
const PageDependencyGraph = require('../../../../gather/computed/page-dependency-graph');

const assert = require('assert');
let nextRequestId = 1;
Expand All @@ -30,7 +31,7 @@ function request(opts) {
function cpuTask({tid, ts, duration}) {
tid = tid || nextTid++;
ts = ts || 0;
const dur = (duration || 0) * 1000 / 5;
const dur = ((duration || 0) * 1000) / 5;
return {tid, ts, dur};
}

Expand Down Expand Up @@ -255,5 +256,32 @@ describe('DependencyGraph/Simulator', () => {
const simulator = new Simulator({serverResponseTimeByOrigin});
assert.throws(() => simulator.simulate(rootNode), /cycle/);
});

describe('on a real trace', () => {
const trace = require('../../../fixtures/traces/progressive-app-m60.json');
const devtoolsLog = require('../../../fixtures/traces/progressive-app-m60.devtools.log.json');
let result;

beforeAll(async () => {
const computedCache = new Map();
const graph = await PageDependencyGraph.request({trace, devtoolsLog}, {computedCache});
const simulator = new Simulator({serverResponseTimeByOrigin});
result = simulator.simulate(graph);
});

it('should compute a timeInMs', () => {
expect(result.timeInMs).toBeGreaterThan(100);
});

it('should sort the task event times', () => {
const nodeTimings = Array.from(result.nodeTimings.entries());

for (let i = 1; i < nodeTimings.length; i++) {
const startTime = nodeTimings[i][1].startTime;
const previousStartTime = nodeTimings[i - 1][1].startTime;
expect(startTime).toBeGreaterThanOrEqual(previousStartTime);
}
});
});
});
});
12 changes: 6 additions & 6 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1951,9 +1951,9 @@
"id": "unminified-javascript",
"title": "Minify JavaScript",
"description": "Minifying JavaScript files can reduce payload sizes and script parse time. [Learn more](https://developers.google.com/speed/docs/insights/MinifyResources).",
"score": 1,
"score": 0.88,
"scoreDisplayMode": "numeric",
"rawValue": 0,
"rawValue": 150,
"displayValue": "Potential savings of 30 KB",
"warnings": [],
"details": {
Expand Down Expand Up @@ -1983,7 +1983,7 @@
"wastedPercent": 42.52388078488413
}
],
"overallSavingsMs": 0,
"overallSavingsMs": 150,
"overallSavingsBytes": 30470
}
},
Expand Down Expand Up @@ -2070,9 +2070,9 @@
"id": "uses-text-compression",
"title": "Enable text compression",
"description": "Text-based resources should be served with compression (gzip, deflate or brotli) to minimize total network bytes. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/text-compression).",
"score": 0.88,
"score": 0.75,
"scoreDisplayMode": "numeric",
"rawValue": 150,
"rawValue": 300,
"displayValue": "Potential savings of 63 KB",
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -2105,7 +2105,7 @@
"wastedBytes": 8442
}
],
"overallSavingsMs": 150,
"overallSavingsMs": 300,
"overallSavingsBytes": 64646
}
},
Expand Down
8 changes: 4 additions & 4 deletions proto/sample_v2_round_trip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,12 +2075,12 @@
}
],
"overallSavingsBytes": 30470.0,
"overallSavingsMs": 0.0,
"overallSavingsMs": 150.0,
"type": "opportunity"
},
"displayValue": "Potential savings of 30\u00a0KB",
"id": "unminified-javascript",
"score": 1.0,
"score": 0.88,
"scoreDisplayMode": "numeric",
"title": "Minify JavaScript",
"warnings": []
Expand Down Expand Up @@ -2452,12 +2452,12 @@
}
],
"overallSavingsBytes": 64646.0,
"overallSavingsMs": 150.0,
"overallSavingsMs": 300.0,
"type": "opportunity"
},
"displayValue": "Potential savings of 63\u00a0KB",
"id": "uses-text-compression",
"score": 0.88,
"score": 0.75,
"scoreDisplayMode": "numeric",
"title": "Enable text compression"
},
Expand Down