core(fr): prepare emulation utilities for shared use #12375

patrickhulce · 2021-04-19T16:41:26Z

Summary
Broken out of #12366, just the emulation shared pieces. We already basically had this split in lib/emulation.js so it was just moving the last two methods that put it together :)

Related Issues/PRs
ref #11313 #12366

brendankenny

Worth bikeshedding for a minute? Maybe not, considering the improvement already here over the status quo :)

But:

emulate()

throttle() // Network and CPU
clearThrottling() // Network and CPU

enableNetworkThrottling()
clearNetworkThrottling()

enableCPUThrottling()
clearCPUThrottling()

enableThrottling() to match clearThrottling/enableNetworkThrottling/enableCPUThrottling? Or were you thinking in terms of matching emulate()?

brendankenny · 2021-04-19T17:37:14Z

lighthouse-core/gather/gather-runner.js

@@ -129,7 +130,8 @@ class GatherRunner {
    log.time(status);
    const resetStorage = !options.settings.disableStorageReset;
    await driver.assertNoSameOriginServiceWorkerClients(options.requestedUrl);
-    await driver.beginEmulation(options.settings);
+    await emulation.emulate(driver.defaultSession, options.settings);
+    await emulation.throttle(driver.defaultSession, options.settings);


this is a literal translation of the existing driver.beginEmulation, but could we not throttle here since per-pass throttling is going to be set regardless? I don't think there's any reason to set it here, at least.

brendankenny · 2021-04-19T17:39:58Z

lighthouse-core/lib/emulation.js

+  // TODO(FR-COMPAT): reconsider if this should be resetting anything
+  if (settings.throttlingMethod !== 'devtools') return clearNetworkThrottling(session);


seems more like a gather-runner concern than an emulation utility. Could we move out to there and keep this a little more separated?

My response here is tightly related to my reflection on the poll so I'll respond to both here.

enableThrottling won the informal poll, so we'll rename throttle to that

Looking at this more closely, I don't think enableThrottling is right. The emulate and throttle methods aren't blindly turning on the emulation/throttling protocol methods, they're looking at config settings and setting what needs to be set according to the config settings.

A new method called enableThrottling that accepts the throttling object from settings that just calls the two makes sense, but at that point honestly why bother having a method at all?

I think your argument @brendankenny is that this logic should move up a layer into gather-runner but one of the goals here explicitly was to extract the shared parts for FR and at least for now all of this should be shared logic. Given that, I'm not sure I agree there's a benefit to having this live one layer up (it's not like we're mixing in the passConfig object like we were previously, there I wholeheartedly agree it shouldn't live in shared lib/).

Looking at this more closely, I don't think enableThrottling is right.

conveniently realized after the vote is closed :P

A new method called enableThrottling that accepts the throttling object from settings that just calls the two makes sense, but at that point honestly why bother having a method at all?

Ha, you wouldn't have to twist my arm on this if this is the direction you actually wanted to go ;) The clear*Throttling methods are basically already there.

I think it's the fact that it's exactly the same condition as if (passConfig.useThrottling) emulation.throttle(); else emulation.clearThrottling(); that ran right before this line that trips me up. If I were writing this as new code but wanted to match current behavior, I would go with if (passConfig.useThrottling && settings.throttlingMethod === 'devtools') ... in the caller in this case (which matches the pass-centric view "in this pass, enable throttling if throttlingMethod is 'devtools' and it's enabled for this pass").

I can easily see the arguments you're making here too, though, so this seems good to go. From the TODO(FR-COMPAT) and WPT-compat comments, those conditions might not end up doing the same thing anyways.

I would go with if (passConfig.useThrottling && settings.throttlingMethod === 'devtools')

~~I'm fine with that. Deal.~~ You had me convinced until I looked at the code again, it's not the same condition! See below

Wait nevermind it's not the same condition, that's why I was putting this fight up in the first place :P

It's super specific that we only clear the network but not the CPU when we're doing other throttlingMethods. If that changes in FR I'm happy to move toward your suggestion, but for now that super specific detail feels like it should stay with throttle

It's super specific that we only clear the network but not the CPU when we're doing other throttlingMethods. If that changes in FR I'm happy to move toward your suggestion, but for now that super specific detail feels like it should stay with throttle

oh right, lord. We need to clear up what's going on with that :)

patrickhulce · 2021-04-19T21:35:28Z

~~enableThrottling won the informal poll, so we'll rename throttle to that~~ but looking closer I really don't think that's an accurate name, see discussion above.

brendankenny

LGTM

brendankenny · 2021-04-20T22:56:38Z

lighthouse-core/lib/emulation.js

+  // TODO(FR-COMPAT): reconsider if this should be resetting anything
+  if (settings.throttlingMethod !== 'devtools') return clearNetworkThrottling(session);


Looking at this more closely, I don't think enableThrottling is right.

conveniently realized after the vote is closed :P

A new method called enableThrottling that accepts the throttling object from settings that just calls the two makes sense, but at that point honestly why bother having a method at all?

Ha, you wouldn't have to twist my arm on this if this is the direction you actually wanted to go ;) The clear*Throttling methods are basically already there.

I think it's the fact that it's exactly the same condition as if (passConfig.useThrottling) emulation.throttle(); else emulation.clearThrottling(); that ran right before this line that trips me up. If I were writing this as new code but wanted to match current behavior, I would go with if (passConfig.useThrottling && settings.throttlingMethod === 'devtools') ... in the caller in this case (which matches the pass-centric view "in this pass, enable throttling if throttlingMethod is 'devtools' and it's enabled for this pass").

I can easily see the arguments you're making here too, though, so this seems good to go. From the TODO(FR-COMPAT) and WPT-compat comments, those conditions might not end up doing the same thing anyways.

core(fr): prepare emulation utilities for shared use

7f0b930

patrickhulce requested a review from a team as a code owner April 19, 2021 16:41

patrickhulce requested review from connorjclark and removed request for a team April 19, 2021 16:41

google-cla bot added the cla: yes label Apr 19, 2021

devtools-bot assigned connorjclark Apr 19, 2021

devtools-bot added the waiting4reviewer label Apr 19, 2021

connorjclark approved these changes Apr 19, 2021

View reviewed changes

brendankenny reviewed Apr 19, 2021

View reviewed changes

patrickhulce assigned brendankenny and unassigned connorjclark Apr 20, 2021

feedback

8efad1d

vercel bot deployed to Preview April 20, 2021 17:40 View deployment

brendankenny approved these changes Apr 20, 2021

View reviewed changes

GoogleChrome deleted a comment Apr 20, 2021

update tests for feedback

a999524

vercel bot deployed to Preview April 21, 2021 16:56 View deployment

patrickhulce merged commit f57ac8c into master Apr 21, 2021

patrickhulce deleted the fr_emulation_cleanup branch April 21, 2021 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(fr): prepare emulation utilities for shared use #12375

core(fr): prepare emulation utilities for shared use #12375

patrickhulce commented Apr 19, 2021

brendankenny left a comment •

edited

Loading

brendankenny Apr 19, 2021

patrickhulce Apr 20, 2021

brendankenny Apr 19, 2021

patrickhulce Apr 20, 2021

brendankenny Apr 20, 2021

patrickhulce Apr 21, 2021 •

edited

Loading

patrickhulce Apr 21, 2021 •

edited

Loading

brendankenny Apr 21, 2021

patrickhulce commented Apr 19, 2021 •

edited

Loading

brendankenny left a comment

brendankenny Apr 20, 2021

		// TODO(FR-COMPAT): reconsider if this should be resetting anything
		if (settings.throttlingMethod !== 'devtools') return clearNetworkThrottling(session);

core(fr): prepare emulation utilities for shared use #12375

core(fr): prepare emulation utilities for shared use #12375

Conversation

patrickhulce commented Apr 19, 2021

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

brendankenny Apr 19, 2021

Choose a reason for hiding this comment

patrickhulce Apr 20, 2021

Choose a reason for hiding this comment

brendankenny Apr 19, 2021

Choose a reason for hiding this comment

patrickhulce Apr 20, 2021

Choose a reason for hiding this comment

brendankenny Apr 20, 2021

Choose a reason for hiding this comment

patrickhulce Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

patrickhulce Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

brendankenny Apr 21, 2021

Choose a reason for hiding this comment

patrickhulce commented Apr 19, 2021 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Apr 20, 2021

Choose a reason for hiding this comment

brendankenny left a comment •

edited

Loading

patrickhulce Apr 21, 2021 •

edited

Loading

patrickhulce Apr 21, 2021 •

edited

Loading

patrickhulce commented Apr 19, 2021 •

edited

Loading