core(driver): request smaller trace in m71+ #6117

paulirish · 2018-09-26T19:39:49Z

Backstory in #3596.

Results: cnn.com's trace goes from 69M to 47M.

--

Using Browser.getVersion and parsing out the chrome milestone. We can augment this pattern later for isMobile and os like @patrickhulce suggested.
I made a lil sendCommand mock response helper in driver-test. There are similar stubs for on() and once().
we now default to the new disabled-by-default-lighthouse category and fallback to 'toplevel', as @brendankenny very wisely suggested
ideally i'd like to drop the SCHEDULABLE_TASK_TITLE possibilities from FOUR down to 2. But it requires updating a lot of fixture data. So we can do that in a followup.

brendankenny

other thoughts:

we could fix the FIXME in driver.getUserAgent and use Browser.getVersion. Then this could be a call to (await driver.getUserAgent()).product instead of putting it on the pass context
if not, and hostUserAgent is just going to be on the pass context until m71, put a TODO?
if you think hostUserAgent on there isn't going to be temporary, should we preparse the UA string it so gatherers can just consult the version number?

brendankenny · 2018-09-26T21:49:10Z

lighthouse-core/gather/driver.js

+    // TODO(COMPAT): Once m71 ships to stable, change the traceCategories for real
+    const reChrome71 = /Chrome\/(Headless)?7[1-9]/;
+    if (passContext.hostUserAgent && reChrome71.test(passContext.hostUserAgent)) {
+      traceCategories.forEach((cat, idx) => {


why forEach? Do we anticipate multiple ones? Maybe this should be changing this._traceCategories. It's very very unlikely, but in case someone is passing in 'toplevel' into additionalTraceCategories and they really mean it :)

could also use map instead of forEach

maybe this should be reversed as well. Have disabled-by-default-lighthouse be the default and replaced by toplevel in Chrome < 71. Then future edits just have to change things here

patrickhulce

so it's just RunTask? was that really all it took? 😲 amazing! 😃

patrickhulce · 2018-09-27T02:10:52Z

lighthouse-core/gather/gather-runner.js

@@ -402,6 +401,7 @@ class GatherRunner {
          // If the main document redirects, we'll update this to keep track
          url: options.requestedUrl,
          settings: options.settings,
+          hostUserAgent: baseArtifacts.HostUserAgent,


I was parsing the user agent over in #5893, maybe after this we should do a refactor for some useful host.chromeVersion, host.isMobile, host.os?

patrickhulce · 2018-09-27T02:16:41Z

I'm mildly nervous about missing some random lantern-impacting event, but I think we've both been through it multiple times before.

paulirish · 2018-10-13T23:09:16Z

ptal

patrickhulce

so exciting :D lookin' real good

patrickhulce · 2018-10-14T19:06:45Z

lighthouse-core/gather/driver.js

+
+    // In Chrome <71, gotta use the chatty 'toplevel' cat instead of our own.
+    // TODO(COMPAT): Once m71 ships to stable, drop this section
+    const milestone = (await this.getBrowserVersion()).milestone; // eg 'Chrome/71.0.3577.0'


it feels like this comment should be up where the regex is that parses out 71 :)

patrickhulce · 2018-10-14T19:08:09Z

lighthouse-core/test/gather/driver-test.js

@@ -46,9 +48,23 @@ function createActiveWorker(id, url, controlledClients, status = 'activated') {
  };
 }

+function createOnceMethodResponse(method, response) {


someday using jests mock functions would be awesome :)

yeah our lame driver/connection mocking everywhere is ripe for a cleanup.

patrickhulce · 2018-10-14T19:08:31Z

lighthouse-core/test/gather/driver-test.js

+    createOnceMethodResponse('Browser.getVersion', m70BrowserVersion);
+
+    // eslint-disable-next-line max-len
+    return driverStub.beginTrace().then(() => {


want to use async/await for new code?

patrickhulce · 2018-10-14T19:09:53Z

lighthouse-core/test/gather/fake-driver.js

-  getUserAgent() {
-    return Promise.resolve('Fake user agent');
+const browserVersion = {
+  protocolVersion: '1.3',


this doesn't return milestone. Is this filename wrong and it's really not a fake driver but a fake connection?

good catch. yeah the separation between protocol response vs driver method response got a little mixed up.

Clarified this now.

patrickhulce

LGTM!

brendankenny

since RunTask is the future, we should also update create-test-trace.js to use it

brendankenny · 2018-10-15T18:31:31Z

lighthouse-core/gather/driver.js

+    const milestone = (await this.getBrowserVersion()).milestone;
+    if (milestone < 71) {
+      const toplevelIndex = traceCategories.indexOf('disabled-by-default-lighthouse');
+      traceCategories.splice(toplevelIndex, 1, 'toplevel');


seems like the splice is unnecessary?

traceCategories[toplevelIndex] = 'toplevel' sgtm

brendankenny · 2018-10-15T18:32:29Z

lighthouse-core/lib/traces/tracing-processor.js

-const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::RunTask';
+const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::DoWork';
+// m65 and earlier
+const SCHEDULABLE_TASK_TITLE_ALT3 = 'TaskQueueManager::ProcessTaskFromWorkQueue';


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

Yes I'd like to redo all our fixture traces. Right now it doesn't seem too bad..
.. I just tested the perf downside of checking the evt name four times and its probably a maximum of ~10ms total on a big trace. So we can ignore that bit.

I think it's aiight for now. But we'll put something in the OKRs about freshening up our fixtures/mocking/test infra.

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.

brendankenny · 2018-10-15T18:34:16Z

lighthouse-core/test/gather/driver-test.js

@@ -46,9 +48,23 @@ function createActiveWorker(id, url, controlledClients, status = 'activated') {
  };
 }

+function createOnceMethodResponse(method, response) {
+  assert.deepEqual(!!sendCommandMockResponses[method], false, 'stub response already defined');


deepEqual left over from an earlier revision?

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.
I can remove though if you don't see the value.

I can remove though if you don't see the value.

sorry, I think it's fine, I meant the deepEqual is kind of overkill because they're both booleans :)

brendankenny · 2018-10-15T18:37:03Z

lighthouse-core/test/gather/driver-test.js

+
+  const mockResponse = sendCommandMockResponses[command];
+  if (mockResponse) {
+    delete sendCommandMockResponses[command];


should use a Map for things like this

I dont see why Map is better, but happy to change it. :)

paulirish · 2018-10-15T19:06:55Z

since RunTask is the future, we should also update create-test-trace.js to use it

sg will do.

paulirish requested a review from patrickhulce as a code owner September 26, 2018 19:39

brendankenny reviewed Sep 26, 2018

View reviewed changes

patrickhulce reviewed Sep 27, 2018

View reviewed changes

brendankenny added the waiting4committer label Oct 4, 2018

paulirish added 2 commits October 11, 2018 19:07

core(driver): request smaller trace in m71+

df59493

feedback

805e714

paulirish force-pushed the adjusttracecats branch from e9a4b6e to 805e714 Compare October 12, 2018 02:38

paulirish added 5 commits October 11, 2018 21:39

fix tests with new createOnceMethodResponse() method

f0535bc

drop the passcontext changes

49caf21

update tracing processor.

651401f

mock

b7ac629

DRY up browserVersion mock usage

6f501bc

paulirish removed the waiting4committer label Oct 13, 2018

paulirish mentioned this pull request Oct 14, 2018

core(driver): reduce required traceCategories again #6271

Merged

patrickhulce suggested changes Oct 14, 2018

View reviewed changes

Feedback

c3e15ca

patrickhulce approved these changes Oct 14, 2018

View reviewed changes

paulirish force-pushed the adjusttracecats branch from 94d8e30 to c3e15ca Compare October 15, 2018 17:34

paulirish merged commit b878616 into master Oct 15, 2018

paulirish deleted the adjusttracecats branch October 15, 2018 17:35

brendankenny reviewed Oct 15, 2018

View reviewed changes

paulirish added a commit that referenced this pull request Oct 15, 2018

core(driver): address bckenny feedback on #6117

434f3e5

paulirish mentioned this pull request Oct 15, 2018

Reduce the size of the trace #3596

Closed

paulirish added a commit that referenced this pull request Oct 16, 2018

core(driver): address bckenny feedback on #6117 (#6285)

72a0932

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(driver): request smaller trace in m71+ #6117

core(driver): request smaller trace in m71+ #6117

paulirish commented Sep 26, 2018 •

edited

Loading

brendankenny left a comment

brendankenny Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018

patrickhulce left a comment

patrickhulce Sep 27, 2018

patrickhulce commented Sep 27, 2018

paulirish commented Oct 13, 2018

patrickhulce left a comment

patrickhulce Oct 14, 2018

patrickhulce Oct 14, 2018

paulirish Oct 14, 2018

patrickhulce Oct 14, 2018

patrickhulce Oct 14, 2018

paulirish Oct 14, 2018

patrickhulce left a comment

brendankenny left a comment

brendankenny Oct 15, 2018

paulirish Oct 15, 2018

brendankenny Oct 15, 2018

paulirish Oct 15, 2018

brendankenny Oct 15, 2018

brendankenny Oct 15, 2018

paulirish Oct 15, 2018

brendankenny Oct 15, 2018

brendankenny Oct 15, 2018

paulirish Oct 15, 2018

paulirish commented Oct 15, 2018

core(driver): request smaller trace in m71+ #6117

core(driver): request smaller trace in m71+ #6117

Conversation

paulirish commented Sep 26, 2018 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Sep 27, 2018

paulirish commented Oct 13, 2018

patrickhulce 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

patrickhulce left a comment

Choose a reason for hiding this comment

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

paulirish commented Oct 15, 2018

paulirish commented Sep 26, 2018 •

edited

Loading

brendankenny Sep 26, 2018 •

edited

Loading