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(throttling): add option to slowdown to a device class #6162

Closed
wants to merge 5 commits into from

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Oct 3, 2018

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
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
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
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
Copy link
Contributor

benschwarz commented Oct 4, 2018

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
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
Copy link
Contributor

benschwarz commented Oct 10, 2018

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
Collaborator Author

@brendankenny @paulirish any opinions here?

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.

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
Member

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
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
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
Member

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
Collaborator Author

bump :)

Copy link
Member

@paulirish paulirish left a comment

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?

Copy link
Member

@exterkamp exterkamp left a comment

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
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
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
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