Skip to content

core(throttling): add option to slowdown to a device class#6162

Closed
patrickhulce wants to merge 5 commits into
masterfrom
cpu_calibration
Closed

core(throttling): add option to slowdown to a device class#6162
patrickhulce wants to merge 5 commits into
masterfrom
cpu_calibration

Conversation

@patrickhulce

@patrickhulce patrickhulce commented Oct 3, 2018

Copy link
Copy Markdown
Collaborator

Summary
First iteration of what #5871 laid the groundwork for, there are quite a few ways of going about doing this, but the determination from before was that we can't do precise 2.353x throttling decisions but we can tell when a device has WAAAAAY different perf characteristics from another and adjust accordingly.

Nothing will change by default, but this PR introduces a new throttling property cpuTargetDeviceClass (@benschwarz we need your sage naming wisdom here 😆), with 4 main device classes based on the data collected for #5871. 8x 4x 2x and 1x on a Macbook Pro. If the host device is in the 2x class, and the target device class is 4x, the multiplier will be reduced to 2x instead of using 4x, and so on.

Related Issues/PRs
#5871

@benschwarz

Copy link
Copy Markdown
Contributor

@patrickhulce given the range of devices people are likely to run LH on, do we need more data provided? I can more than likely help there

@benschwarz

Copy link
Copy Markdown
Contributor

This looks really good so far @patrickhulce - The only thing that I'm 🤔 about is how people will remember what very-slow actually means? Could we class them in some other way? (I thought of years, "-1 year" -- like how people use autoprefixer etc… but that doesn't feel right, I think)

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

given the range of devices people are likely to run LH on, do we need more data provided? I can more than likely help there

Sure the more the better! The most important thing IMO would be selecting cutoffs that will affect the fewest people. i.e. there's the fewest devices on the border of two categories.

how people will remember what very-slow actually means? Could we class them in some other way?

In the original throttling doc I wrote I had used something more like "Premium Mobile", "Budget Mobile", etc, but given the disparity between premium android and premium iOS it's not exactly accurate or fair. Year is also tricky because new budget android devices (for example Alcatel 1X) are much slower than phones from 5 years ago. Definitely on board with finding something as descriptive/accurate as possible but I'm at a loss for what that actually is haha 🤷‍♀️

@benschwarz

benschwarz commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Definitely on board with finding something as descriptive/accurate as possible but I'm at a loss for what that actually is haha 🤷‍♀️

I've the same feels atm. Let's sit on it for a day or two and see if anything falls out?

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

Let's sit on it for a day or two and see if anything falls out?

So now that it's 7 days, has anything fallen out of anyone? ;)

@benschwarz

benschwarz commented Oct 10, 2018

Copy link
Copy Markdown
Contributor

I think from what we've seen, professional performance advocates will do the research into the devices and choose something that works for the circumstances that that are being tested for, whereas "performance interested" folks are more likely to take our guidance on what the test devices are.

Given that, I think the best naming convention that comes to mind is exactly what you've come up with.

I'd love to see a table of the kinds of devices that each setting represents, something that can be referenced and updated in (likely a year or so?). Maybe with an accompanying benchmarkIndex for completeness:

Setting Benchmark index range Devices
very-slow 0—100 Alcatel 1C
slow 100—500 Motorola G3
medium 500—700 iPhone 6
fast > 1000 iPhone X
none > 1250 Late model Macbook

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

@brendankenny @paulirish any opinions here?

@brendankenny brendankenny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

immediate thoughts:

  • ideally we'd only have one way to set the CPU multiplier. Does anyone use cpuSlowdownMultiplier except DevTools? We could get rid of it with v4 (internally we can use whatever form(s) we want to, of course)
  • we try not to alter settings after being passed in (I can't say for sure we don't :). Since this should be just for emulation at gathering time and the rest of the time used in audits for lantern stuff (and assuming eliminating cpuSlowdownMultiplier re: above), could we have an emulation property for this in gather-runner and have the rest be based on an artifact?

@brendankenny

Copy link
Copy Markdown
Contributor

Does anyone use cpuSlowdownMultiplier except DevTools? We could get rid of it with v4

just to be clear, I mean get rid of it and replace it with cpuTargetDeviceClass selection instead :)

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

I definitely like all the things you're saying. Treating this as a computed artifact for the real multiplier SGTM assuming the other pieces look good to folks to proceed.

I'm not convinced we can eliminate cpuSlowdownMultiplier though :/ I think the strongest use case for keeping it is in LR/DAZL-like situations when you have a fleet of servers you want to run lots of tests with exact same settings and not waste time running the benchmark every time (yet to be implemented :D). I see a future world in which we only run the benchmark when cpuTargetDeviceClass is set.

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

The other of the stagnant PRs I mentioned today :)

This would be nice to clear up before v4 if our decision involves eliminating cpuSlowdownMultiplier (I'm not sure we want to do that), but otherwise can wait for post-CDS.

@brendankenny

Copy link
Copy Markdown
Contributor

Yeah, your point above is a good one, and even if do get more confident that we can eliminate cpuSlowdownMultiplier, we'll likely want a decent transition time anyways as we tune the benchmark, interpretation of the results, etc.

Makes sense to start with it as just additive.

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

bump :)

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This works for me.

Title of the PR is a little misleading though. Feel more like... `core(throttling): add option to slowdown to a device class" or sumpthin.

The plan is to try this out and see how it goes before setting the multiplier dynamically for everyone, right?

@exterkamp exterkamp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM
I know this is a preliminary PR and isn't really applied yet, but I was thinking about how we might surface this to the user to let them know that the benchmark index has been adjusted, some ideas:
image
image
image
Or just the final device class in wording:
image

@patrickhulce patrickhulce changed the title core(throttling): add automatic multiplier setting core(throttling): add option to slowdown to a device class Jan 14, 2019
@patrickhulce

Copy link
Copy Markdown
Collaborator Author

Feel more like... `core(throttling): add option to slowdown to a device class" or sumpthin.

done!

The plan is to try this out and see how it goes before setting the multiplier dynamically for everyone, right?

yep

I was thinking about how we might surface this to the user to let them know that the benchmark index has been adjusted, some ideas:

Oooh I like those. I was also thinking it might be part of a much larger rework of the environment settings. Something that communicates more clearly how it was run and what we were targeting.

@benschwarz

Copy link
Copy Markdown
Contributor

I know this is a preliminary PR and isn't really applied yet, but I was thinking about how we might surface this to the user to let them know that the benchmark index has been adjusted, some ideas:

I prefer your first option @exterkamp — It's important that people can clearly see what the machine was rated at, as well as the slowing applied to it 👍

@patrickhulce

Copy link
Copy Markdown
Collaborator Author

This isn't ready for primetime, and we've decided to revisit this with perhaps a new benchmark in the future.

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.

5 participants