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.

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(config): switch to throttling settings object #4879

Merged
merged 6 commits into from
Mar 29, 2018

Conversation

patrickhulce
Copy link
Collaborator

ok followup to ongoing work for config settings, I had to draw the line somewhere to keep this reasonable size, so in this PR it simply removes the two flags and changes our existing logic to use the throttling object instead.

upcoming PRs will have the 'simulate' throttlingMethod to actually return simulated metrics, but already at this stage you can do awesome stuff like lighthouse --throttling.requestLatency 2000 😃

I started to do the other constants cleanup here, but it started to balloon the PR

closes #4871

@patrickhulce
Copy link
Collaborator Author

note this is also blocking lots of pending PRs so timely 🕙reviews please! don't want to forced to start stuffing my followup work turning this into a mega PR 😆

return driver.sendCommand('Emulation.setCPUThrottlingRate', CPU_THROTTLE_METRICS);
/**
* @param {Driver} driver
* @param {LH.ThrottlingSettings|undefined} throttlingSettings
Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 28, 2018

Choose a reason for hiding this comment

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

@brendankenny for some reason type checking was refusing to allow undefined as an input to LH.ThrottlingSettings=, no clue why, any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

looks like this is a temporary bug in the version of TS we're using. I was actually pleased that it started completely differentiating between optional and |undefined, but looks like that wasn't on purpose :) (there's a separate discussion around using void or some other kind of missing type to indicate the "optional but never undefined" case: microsoft/TypeScript#13195).

Later releases have that fixed, though. I'm about to make a PR bumping us to typescript@2.9.0-dev.20180323 (for reasons I'll explain there). You could get a jump on that.

Copy link
Collaborator

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Nothing to add 🙄, not really helping but at least you got a review out of me 😄

Did we change base throttling as travis is failing?

@patrickhulce
Copy link
Collaborator Author

Did we change base throttling as travis is failing?

whoops good catch, didn't push my latest config tweak :D

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Still reviewing, but some initial comments. I think this is going to be a real good change

throttlingMethod: 'devtools',
throttling: {
requestLatency: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.latency,
downloadThroughput: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.downloadThroughput * 8,
Copy link
Member

Choose a reason for hiding this comment

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

this bits vs bytes thing is unfortunate to keep track of. Should we put the units in the property name? Or any other ideas how to simplify/keep it clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding the units in the name isn't the worst if we keep the units reasonable, essentially everything throughput should be in bits until we have to translate to DevTools :)

requestLatencyMs
downloadThroughputKbps
uploadThroughputKbps

const throttleNetwork = passConfig.useThrottling && !settings.disableNetworkThrottling;
const cpuPromise = throttleCpu ?
emulation.enableCPUThrottling(this) :
if (settings.throttlingMethod !== 'devtools') {
Copy link
Member

Choose a reason for hiding this comment

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

we use setThrottling for goOnline and goOffline, so does this break that for non devtools throttling? We may need to separate those from this anyways as passConfig.useThrottling probably shouldn't affect a call to driver.goOffline() anyways.

We can kick that to another PR if it gets involved, but is there another quick fix in the meantime?

Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 28, 2018

Choose a reason for hiding this comment

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

Ah yeah good catch, we can go offline but never go back online atm if devtools throttling is off or useThrottling is false :), I'll clean this up it's a pretty easy fix

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 and tests added!

return driver.sendCommand('Emulation.setCPUThrottlingRate', CPU_THROTTLE_METRICS);
/**
* @param {Driver} driver
* @param {LH.ThrottlingSettings|undefined} throttlingSettings
Copy link
Member

Choose a reason for hiding this comment

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

looks like this is a temporary bug in the version of TS we're using. I was actually pleased that it started completely differentiating between optional and |undefined, but looks like that wasn't on purpose :) (there's a separate discussion around using void or some other kind of missing type to indicate the "optional but never undefined" case: microsoft/TypeScript#13195).

Later releases have that fixed, though. I'm about to make a PR bumping us to typescript@2.9.0-dev.20180323 (for reasons I'll explain there). You could get a jump on that.

function enableNetworkThrottling(driver) {
return driver.sendCommand('Network.emulateNetworkConditions', TYPICAL_MOBILE_THROTTLING_METRICS);
/**
* @param {Driver} driver
Copy link
Member

Choose a reason for hiding this comment

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

for type checking in this file we'll have to require Driver just to get the type (with an eslint ignore unused above it). Any thoughts on that style? It's pretty ugly but should have no real impact on a node run or browserify (and we obviously use driver so it won't ever be tree shaken if we switch bundling tools).

OTOH there's exposing the major types on LH so it could be {LH.Driver}, but that has its own ugliness (it's a real JS class, not an interface like the rest of our typings) and I have no idea what the impact on tsc performance will be as we continue to expand the global type graph :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah sg, I've used that before 👍

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Except for the one Kbps question, LGTM. Great cleanup :)

@@ -3,9 +3,12 @@
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
// @ts-nocheck
Copy link
Member

Choose a reason for hiding this comment

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

🎉

// Injecting this function clientside is no longer neccessary as of m62. This is done
// on the backend when `Emulation.setTouchEmulationEnabled` is set.
// https://bugs.chromium.org/p/chromium/issues/detail?id=133915#c63
// Once m62 hits stable (~Oct 20th) we can nuke this entirely
Copy link
Member

Choose a reason for hiding this comment

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

:)

break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
const down = byteToKbit(throttling.downloadThroughputKbps);
Copy link
Member

Choose a reason for hiding this comment

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

wait, shouldn't these already be Kbps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops yes good catch forgot to update :)

'cpuThrottling': `${CPU_THROTTLE_METRICS.rate}x slowdown`,
'networkThrottling': `${latency}ms RTT, ${byteToMbit(downloadThroughput)}Mbps down, ` +
`${byteToMbit(uploadThroughput)}Mbps up`,
deviceEmulation: settings.disableDeviceEmulation ? 'Disabled' : 'Nexus 5X',
Copy link
Member

@brendankenny brendankenny Mar 29, 2018

Choose a reason for hiding this comment

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

👍 way better than the old enabled/disabled and a decription thing


// same multiplier as Lighthouse uses for CPU emulation
const DEFAULT_CPU_TASK_MULTIPLIER = emulation.CPU_THROTTLE_METRICS.rate;
// layout tasks tend to be less CPU-bound and do not experience the same increase in duration
const DEFAULT_LAYOUT_TASK_MULTIPLIER = DEFAULT_CPU_TASK_MULTIPLIER / 2;
const DEFAULT_LAYOUT_TASK_MULTIPLIER = .5;
Copy link
Member

Choose a reason for hiding this comment

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

didn't we agree earlier we prefer a leading 0? 0.5

VERY BIG DEAL. not a nit whatsoever. 😠

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha sure done

downloadThroughput: throttlingSettings.downloadThroughputKbps || 0,
uploadThroughput: throttlingSettings.uploadThroughputKbps || 0,
};
} else {
Copy link
Member

Choose a reason for hiding this comment

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

what case is this handling? if throttlingSettings are provided via config now, what is this fallback case for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for configs that don't expressly set throttling or extend the default, also tests :)

Copy link
Member

Choose a reason for hiding this comment

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

this could be one of the things we (eventually) pull into config.js so once it comes out throttling.settings is always defined

@paulirish paulirish removed the request for review from vinamratasingal-zz March 29, 2018 20:25
@patrickhulce patrickhulce merged commit 467453d into master Mar 29, 2018
@patrickhulce patrickhulce deleted the switch_to_throttling_method branch March 29, 2018 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace throttling flags with throttlingMethod
5 participants