-
Notifications
You must be signed in to change notification settings - Fork 9
Add more parameters and wait for server availability #18
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?
Conversation
|
LGTM! |
|
Hmm, I feel conflicted about this. It clearly makes the scripts more powerful, but I wonder if it makes them less useful for the intended purposes:
I know the defaults mean users can still just invoke-and-go, but the extra capability does reduce the readability of the scripts. It means if we're doing a live demo and we do a If we wanted to do things 'properly', wouldn't we use the 'medium-complexity' scripts that Eric is working on? If the crappy scripts are actually ok, it leaves less of a gap for the medium scripts to fit into. :) Did you happen to spot how much of a difference waiting for the first request makes to the throughput? I guess not doing so would penalise the runtime with the slower start time and thus be 'unfair'. I can't decide if the extra complexity is worth it for the fairness, or not. I think on that one it probably is, but for the parameters, I'd almost just want to say people should edit the script if they don't like the defaults, because it's only a simple shell script. We could maybe give variable names to the arguments, though. That does make it more obvious what's going on with a So my initial take is
|
|
I have mixed feelings as well. |
Two purposes:
In order to feel trustworthy, either to an audience or to someone exploring on their laptop, our scripts have to be easy to understand and digest, which means they'd ideally be one or two lines, and not use any unfamiliar tools. We know they'll actually be less valid if they're that simple, but that's why we have the 'ok, now do it properly' version, and the surrounding discussion about 'here's what's wrong with the numbers you just got'. But there's no point in having two 'do it properly' sets of scripts. :) |
Thanks, got it! Now, let's say we are at a conference, using this script, and it reports bad numbers (which is possible): it requires to the speaker, live, to "fix" it, in order to obtain something better. A third option, which is the purpose of this PR, is to have a slightly more complex script which doesn't need to be fixed live, but just configured, to obtain "good enough" numbers, making it
At the same time, by making it configured by default to be broken, will make easier to show how numbers can get more reliable, by changing some parameters values. Said that, I could remove:
And see how it looks like. |
0061dcd to
c8d5d8e
Compare
|
I've tried to reduce the visual noise and still allow a speaker to tune more easily the script |
|
this is why |
c8d5d8e to
4e57fd9
Compare
|
last but not least: having a way to parametrize the number of cores is good to show people how performance can be affected in some unexepected ways i.e. single runtime core can silently switch the GC algorithm (as well as scaling the number of compiler threads) ^^ |
IDK @holly-cummins if that defeat the purpose of simplicity but it should be opaque to users which run the default values.
If you like the type of changes I can do the same for the other scripts (that's why is in DRAFT).