-
Notifications
You must be signed in to change notification settings - Fork 88
Add complexity param #430
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
base: main
Are you sure you want to change the base?
Add complexity param #430
Conversation
Without looking closely at the code change, I think this is a neat idea and I could imagine us increasing the complexity of charts, text editing, etc if we move forward with it. Will be curious for feedback from @julienw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea :-)
resources/developer-mode.mjs
Outdated
@@ -101,11 +102,21 @@ function createUIForSyncStepDelay() { | |||
return label; | |||
} | |||
|
|||
function createTimeRangeUI(labelText, initialValue, unit = "ms", min = 0, max = 1000) { | |||
function createUIForComplexity() { | |||
const { range, label } = createTimeRangeUI("Relative complexity", params.complexity, "x", 0, 10, 0.01); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that rather than a float, maybe this can be an integer between 1 and 100, with 50 being the default?
Then later you can divide this number by 50 to get the multiplier.
And you can cut down a lot of the changes.
But this is merely a suggestion, I'm also happy with what you did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda prefer a normalized float value, aiming quite literaly at a complexity-factor, 2x => it should run roughly twice as slow.
…10-09_complexity_param
I've now changed the complexity slider to have an exponential scale which makes it a bit more comfortable to use. |
…10-09_complexity_param
…10-09_complexity_param
…10-09_complexity_param
✅ Deploy Preview for webkit-speedometer-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
can we also add the complexity value to the json?
return label; | ||
} | ||
|
||
function createExpRangeUI(labelText, paramKey, unit = "ms", min = 0, max = 1000, step = 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit afraid of the accessibility, but this looks mostly OK.
Only the accessibility name on the <label>
itself is a bit off, but on the slider this looks fine, so I think we're pretty good. I'm not sure why the slider's label is fine but not the label's label...
We should probably log all non-standard params to the json file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, looks good to me
…10-09_complexity_param
Depends on params.mjs changes in #469
Work in progress proposal for adding a variable complexity param.
complexity === 1.0
: workloads run in default configurationcomplexity > 1.0
: ideally workloads run proportionally slowercomplexity < 1.0
: ideally workloads run proportionally fastBackground:
We're occasionally seeing issues with gc timing / optimisations heuristics with the current workloads. If we can dynamically change the duration / complexity of workloads it's a bit easier to assess how good these heuristics are and how much we're potentially overfitting.
Current Limitations: