-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(driver): request smaller trace in m71+ #6117
Changes from all commits
df59493
805e714
f0535bc
49caf21
651401f
b7ac629
6f501bc
c3e15ca
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 |
---|---|---|
|
@@ -8,12 +8,16 @@ | |
// The ideal input response latency, the time between the input task and the | ||
// first frame of the response. | ||
const BASE_RESPONSE_LATENCY = 16; | ||
// m65 and earlier | ||
const SCHEDULABLE_TASK_TITLE = 'TaskQueueManager::ProcessTaskFromWorkQueue'; | ||
// m71+ We added RunTask to `disabled-by-default-lighthouse` | ||
const SCHEDULABLE_TASK_TITLE_LH = 'RunTask'; | ||
// m69-70 DoWork is different and we now need RunTask, see https://bugs.chromium.org/p/chromium/issues/detail?id=871204#c11 | ||
const SCHEDULABLE_TASK_TITLE_ALT1 = 'ThreadControllerImpl::RunTask'; | ||
// In m66-68 refactored to this task title, https://crrev.com/c/883346 | ||
const SCHEDULABLE_TASK_TITLE_ALT1 = 'ThreadControllerImpl::DoWork'; | ||
// m69+ DoWork is different and we now need RunTask, see https://bugs.chromium.org/p/chromium/issues/detail?id=871204#c11 | ||
const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::RunTask'; | ||
const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::DoWork'; | ||
// m65 and earlier | ||
const SCHEDULABLE_TASK_TITLE_ALT3 = 'TaskQueueManager::ProcessTaskFromWorkQueue'; | ||
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. have you thought about a long-term support plan for these? Supporting old trace versions seems pretty good if it's not much work, but otoh our test traces are increasingly not looking like traces from master 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. Yes I'd like to redo all our fixture traces. Right now it doesn't seem too bad.. I think it's aiight for now. But we'll put something in the OKRs about freshening up our fixtures/mocking/test infra. 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. yeah, definitely not a performance issue (string compares are fast, especially if they haven't ever been modified), and since the events all more or less cover the subset of events we're interested in (we only have to do the string check, not more convoluted fixes to support each), it's more a case of being able to easily see what the state of the world is in the code. The downside for updating fixtures (besides being really annoying to do) is we get less coverage of the old versions, so we may want to really phase out support completely for some of these if we actually do that. |
||
|
||
|
||
const LHError = require('../lh-error'); | ||
|
||
class TraceProcessor { | ||
|
@@ -234,9 +238,10 @@ class TraceProcessor { | |
* @return {boolean} | ||
*/ | ||
static isScheduleableTask(evt) { | ||
return evt.name === SCHEDULABLE_TASK_TITLE || | ||
evt.name === SCHEDULABLE_TASK_TITLE_ALT1 || | ||
evt.name === SCHEDULABLE_TASK_TITLE_ALT2; | ||
return evt.name === SCHEDULABLE_TASK_TITLE_LH || | ||
evt.name === SCHEDULABLE_TASK_TITLE_ALT1 || | ||
evt.name === SCHEDULABLE_TASK_TITLE_ALT2 || | ||
evt.name === SCHEDULABLE_TASK_TITLE_ALT3; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,12 +6,14 @@ | |
'use strict'; | ||
|
||
let sendCommandParams = []; | ||
let sendCommandMockResponses = {}; | ||
|
||
const Driver = require('../../gather/driver.js'); | ||
const Connection = require('../../gather/connections/connection.js'); | ||
const Element = require('../../lib/element.js'); | ||
const assert = require('assert'); | ||
const EventEmitter = require('events').EventEmitter; | ||
const {protocolGetVersionResponse} = require('./fake-driver'); | ||
|
||
const connection = new Connection(); | ||
const driverStub = new Driver(connection); | ||
|
@@ -46,9 +48,23 @@ function createActiveWorker(id, url, controlledClients, status = 'activated') { | |
}; | ||
} | ||
|
||
function createOnceMethodResponse(method, response) { | ||
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. someday using jests mock functions would be awesome :) 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. yeah our lame driver/connection mocking everywhere is ripe for a cleanup. |
||
assert.deepEqual(!!sendCommandMockResponses[method], false, 'stub response already defined'); | ||
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.
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. I wrote the method assuming someone else may try to use it. And just wanted to assert no one was going to use it twice. 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.
sorry, I think it's fine, I meant the |
||
sendCommandMockResponses[method] = response; | ||
} | ||
|
||
connection.sendCommand = function(command, params) { | ||
sendCommandParams.push({command, params}); | ||
|
||
const mockResponse = sendCommandMockResponses[command]; | ||
if (mockResponse) { | ||
delete sendCommandMockResponses[command]; | ||
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. should use a Map for things like this 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. I dont see why Map is better, but happy to change it. :) |
||
return Promise.resolve(mockResponse); | ||
} | ||
|
||
switch (command) { | ||
case 'Browser.getVersion': | ||
return Promise.resolve(protocolGetVersionResponse); | ||
case 'DOM.getDocument': | ||
return Promise.resolve({root: {nodeId: 249}}); | ||
case 'DOM.querySelector': | ||
|
@@ -99,6 +115,7 @@ connection.sendCommand = function(command, params) { | |
describe('Browser Driver', () => { | ||
beforeEach(() => { | ||
sendCommandParams = []; | ||
sendCommandMockResponses = {}; | ||
}); | ||
|
||
it('returns null when DOM.querySelector finds no node', () => { | ||
|
@@ -232,16 +249,33 @@ describe('Browser Driver', () => { | |
}); | ||
|
||
it('will use requested additionalTraceCategories', () => { | ||
return driverStub.beginTrace({additionalTraceCategories: 'v8,v8.execute,toplevel'}).then(() => { | ||
return driverStub.beginTrace({additionalTraceCategories: 'loading,xtra_cat'}).then(() => { | ||
const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start'); | ||
const categories = traceCmd.params.categories; | ||
assert.ok(categories.includes('blink'), 'contains default categories'); | ||
assert.ok(categories.includes('v8.execute'), 'contains added categories'); | ||
assert.ok(categories.indexOf('toplevel') === categories.lastIndexOf('toplevel'), | ||
assert.ok(categories.includes('blink.user_timing'), 'contains default categories'); | ||
assert.ok(categories.includes('xtra_cat'), 'contains added categories'); | ||
assert.ok(categories.indexOf('loading') === categories.lastIndexOf('loading'), | ||
'de-dupes categories'); | ||
}); | ||
}); | ||
|
||
it('will adjust traceCategories based on chrome version', async () => { | ||
// m70 doesn't have disabled-by-default-lighthouse, so 'toplevel' is used instead. | ||
const m70ProtocolGetVersionResponse = Object.assign({}, protocolGetVersionResponse, { | ||
product: 'Chrome/70.0.3577.0', | ||
// eslint-disable-next-line max-len | ||
userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3577.0 Safari/537.36', | ||
}); | ||
createOnceMethodResponse('Browser.getVersion', m70ProtocolGetVersionResponse); | ||
|
||
// eslint-disable-next-line max-len | ||
await driverStub.beginTrace(); | ||
const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start'); | ||
const categories = traceCmd.params.categories; | ||
assert.ok(categories.includes('toplevel'), 'contains old toplevel category'); | ||
assert.equal(categories.indexOf('disabled-by-default-lighthouse'), -1, 'excludes new cat'); | ||
}); | ||
|
||
it('should send the Network.setExtraHTTPHeaders command when there are extra-headers', () => { | ||
return driverStub.setExtraHTTPHeaders({ | ||
'Cookie': 'monster', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,19 @@ | |
*/ | ||
'use strict'; | ||
|
||
module.exports = { | ||
getUserAgent() { | ||
return Promise.resolve('Fake user agent'); | ||
// https://chromedevtools.github.io/devtools-protocol/tot/Browser#method-getVersion | ||
const protocolGetVersionResponse = { | ||
protocolVersion: '1.3', | ||
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. this doesn't return 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. good catch. yeah the separation between protocol response vs driver method response got a little mixed up. Clarified this now. |
||
product: 'Chrome/71.0.3577.0', | ||
revision: '@fc334a55a70eec12fc77853c53979f81e8496c21', | ||
// eslint-disable-next-line max-len | ||
userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3577.0 Safari/537.36', | ||
jsVersion: '7.1.314', | ||
}; | ||
|
||
const fakeDriver = { | ||
getBrowserVersion() { | ||
return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71})); | ||
}, | ||
getBenchmarkIndex() { | ||
return Promise.resolve(125.2); | ||
|
@@ -72,3 +82,6 @@ module.exports = { | |
return Promise.resolve(); | ||
}, | ||
}; | ||
|
||
module.exports = fakeDriver; | ||
module.exports.protocolGetVersionResponse = protocolGetVersionResponse; |
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.
seems like the splice is unnecessary?
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.
traceCategories[toplevelIndex] = 'toplevel'
sgtm