-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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.
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
core(lantern): move metrics to computed artifacts #4766
Changes from 9 commits
f7c0324
43e6ffe
5f34338
17be96d
be7b1d3
d4d2b5f
0be326d
9d64fa9
6a75bf8
413ec5d
ad61451
4c8cdc4
d794222
d8296eb
4694b1f
25e0650
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,8 @@ | |
'use strict'; | ||
|
||
const Audit = require('../audit'); | ||
const PredictivePerf = require('../predictive-perf'); | ||
const ConsistentlyInteractive = require('../../gather/computed/metrics/lantern-consistently-interactive'); // eslint-disable-line max-len | ||
const NetworkAnalysis = require('../../gather/computed/network-analysis'); | ||
const LoadSimulator = require('../../lib/dependency-graph/simulator/simulator.js'); | ||
|
||
const KB_IN_BYTES = 1024; | ||
|
@@ -120,8 +121,8 @@ class UnusedBytes extends Audit { | |
}); | ||
|
||
const savingsOnTTI = Math.max( | ||
PredictivePerf.getLastLongTaskEndTime(simulationBeforeChanges.nodeTiming) - | ||
PredictivePerf.getLastLongTaskEndTime(simulationAfterChanges.nodeTiming), | ||
ConsistentlyInteractive.getLastLongTaskEndTime(simulationBeforeChanges.nodeTiming) - | ||
ConsistentlyInteractive.getLastLongTaskEndTime(simulationAfterChanges.nodeTiming), | ||
0 | ||
); | ||
|
||
|
@@ -135,7 +136,11 @@ class UnusedBytes extends Audit { | |
* @return {!AuditResult} | ||
*/ | ||
static createAuditResult(result, graph) { | ||
const simulatorOptions = PredictivePerf.computeRTTAndServerResponseTime(graph); | ||
const records = []; | ||
graph.traverse(node => node.record && records.push(node.record)); | ||
const simulatorOptions = NetworkAnalysis.computeRTTAndServerResponseTime(records); | ||
// TODO: use rtt/throughput from config.settings instead of defaults | ||
delete simulatorOptions.rtt; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. set to undefined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately that won't have the same effect, gotta love JS key existence vs. undefined biting us left and right 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably keep an eye on performance then, but hopefully properties on simulatorOptions aren't the bottleneck anyways... :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤣 🤣 🤣 |
||
// TODO: calibrate multipliers, see https://github.com/GoogleChrome/lighthouse/issues/820 | ||
Object.assign(simulatorOptions, {cpuTaskMultiplier: 1, layoutTaskMultiplier: 1}); | ||
const simulator = new LoadSimulator(graph, simulatorOptions); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to half delete the
!
here? :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full 🗑 now!