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

Enhancement to arrival rate executors #1386

Open
mstoykov opened this issue Apr 3, 2020 · 6 comments
Open

Enhancement to arrival rate executors #1386

mstoykov opened this issue Apr 3, 2020 · 6 comments
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 executors feature

Comments

@mstoykov
Copy link
Collaborator

mstoykov commented Apr 3, 2020

All of the below might be relevant to other executors not mentioned

Less goroutines starts

As suggested in this comment it might be better if instead of constantly starting goroutines for each iteration, instead to have each VU in a "permanent" goroutine and send it iteration to start.
This will probably be more performant but has the downside that unlike the current approach doesn't guarantee that the least used VU will do the next iteration ... which might be something we care about a lot.

I propose we try it out and benchmark it .. if it is really a lot faster and has some amount of balancing - I am for using it.

Bucketing iterations

While discussing how accurate the proposed start of iterations we came to the idea that having a way to be ... "less" accurate is a good idea. A lot of times people have asked if they can start a number of iterations at the same time and it is also the way that probably some people will understand the arrival rate executors: you start 10 iterations at the beginning of each second. instead of you start 10 iterations over 1 second. The proposal is that we add yet another argument which is in what buckets we should start iterations (I guess we can also use it for variable looping VUs to start VUs in buckets :) ).

So :

execution:{
       "var1": {
            type: "variable-arrival-rate",
            startRate: 50,
            timeUnit: "1s",
            timeBucket: "100ms",
            preAllocatedVUs: 20,
            stages: [
                { target: 50, duration: "5s"},
                { target: 100, duration: "5s", timeBucket "500ms"},
                { target: 100, duration: "5s" },
            ],
        },
}

Would mean that for the first 5 seconds instead of starting one iteration each 1s/50 = 20ms. We will start 5 iterations each 100ms (the time bucket).
Then instead of ramping up (linearly from 50 to 100), which usually would've meant that will be starting iterations every 20ms-10ms depending on how close to the end of the ramp-up we are we will start all the iterations for each 500ms at the same time every 500ms.
Finally instead of doing 1 iteration every 10ms(1s/100) we will start10 iterations every 100ms (the timeBucket).

I wonder if this will not be easier with defining a number of iterations instead of time ... maybe both could be supported.

Maybe we should add another executor for this instead, as this will probably worsen the usual performance? Although it is important to note that the original proposal was to do this by default for 1ms so we don't need to sleep so much .. and possibly be nearly as accurate ... so all of those need to be tested :D

@mstoykov mstoykov added feature evaluation needed proposal needs to be validated or tested before fully implementing it in k6 executors labels Apr 3, 2020
@na--
Copy link
Member

na-- commented Apr 6, 2020

Not sure I like timeBucket as a name, but cadence is the only other think I can think of, and it also doesn't seem very good 😞

I wonder if this will not be easier with defining a number of iterations instead of time ... maybe both could be supported.

I don't think we need this right now, since timeBucket/cadence/whatever would likely be sufficient and better. And if it's not, users can easily wrap the iteration in a for loop themselves, even now. So, I think we shouldn't do it, for now. If it turns out it is needed, after all, we can easily add it as an option at any later point in time, without any breaking changes, since its default value will just be 1.

@na-- na-- changed the title Enhancement to varriable arrival rate executors Enhancement to arrival rate executors Apr 13, 2020
@na--
Copy link
Member

na-- commented Apr 13, 2020

As mentioned in #1285 (comment), the initialization of unplanned VUs probably shouldn't happen in the for { select {} } loop that starts iterations, since it can significantly derail the iteration pace, for both arrival-rate executors.

On the other hand, we probably also don't want to initialize more than 1 unplanned VU at a time. So this feels like it should be a small "side-car" helper to these executors. And it will probably be easier to do with the "Less goroutines starts" implementation from #1386 (comment)

Edit: somewhat connected issue to this: #1407

@na--
Copy link
Member

na-- commented Jul 2, 2020

I started refactoring both arrival-rate executors to use a common execution code, but quickly hit various issues and decided to just copy-paste the fixes for #1386 (comment) (i.e. the newer commits in #1500) for now... 😞

We should still do it before we implement the features proposed above, but yeah, decided to leave it for 0.28.0 as well...

Another thing we should do when refactoring this to have a common run framework is to mock time (#1357 (comment)). This would allow us to test everything in a non-flaky way, finally... And given that we'd need to plug different timers from the two executors, it should be pretty natural to do it in a way that's testable, 🤞

na-- added a commit that referenced this issue Jul 6, 2020
This was an initial attempt to solve the first point from #1514, but there were a few blockers:
- #1499
- #1427
- #1386 (comment)
@mstoykov
Copy link
Collaborator Author

mstoykov commented Jul 7, 2020

8c25045 here are my tries at refactoring the ramping arrival rate into something more ... split up, with the idea to use it in the constant arrival rate as well

@na-- na-- modified the milestones: v0.28.0, v0.29.0 Sep 9, 2020
@na-- na-- modified the milestones: v0.29.0, v0.31.0 Nov 3, 2020
@na-- na-- removed this from the v0.31.0 milestone Jan 26, 2021
@mstoykov
Copy link
Collaborator Author

mstoykov commented Feb 17, 2021

Sparked by this comment in the community forum I did some profiling with the following scripts:

exports.options = {
    scenarios: {
        test1: {
            executor: 'ramping-arrival-rate',
            preAllocatedVUs: 100,
            stages:[
                {target:100000, duration:'0'},
                {target:100000, duration:'6000s'},
            ],

        },
    },
};

exports.default =  function () {}

and

exports.options = {
    scenarios: {
        test1: {
            executor: 'constant-arrival-rate',
            preAllocatedVUs: 1000,
            rate: 100000,
            duration: "6000s"
        },
    },
};

exports.default =  function () {}

Both doing empty iterations with arrival-rate executors
Both hit a limit of around 8-9k (sometimes lower). Running them with 100 instead of 1000 VUs makes this a lot better by them now going as high as 70k. Regardless of that all of them spend an obscene amount of time creating stacks which is likely due to the fact we create a new goroutine per each iteration for arrival-rate executors.
image

There is still more investigation needed to figure out why having 100 VUs is so much better than having 1k, but arrival-rate definitely need at least the "Less goroutines starts" part of this issue as a simple -u 100 -d 10m script can do close to 450k (again having 1000 VUs is worse, but in this case by like 10k in the two tests I did)

@mstoykov mstoykov added this to the v0.32.0 milestone Feb 17, 2021
@na-- na-- modified the milestones: v0.32.0, v0.33.0 Apr 2, 2021
@na--
Copy link
Member

na-- commented Apr 2, 2021

I created a new issue for tracking the performance optimization: #1944

So this issue will just be left for discussing the time-bucketing and other improvements like refactoring the long and complicated Run() method and/or refactoring it even further and re-using the ramping implementation for the constant-arrival-rate executor as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 executors feature
Projects
None yet
Development

No branches or pull requests

2 participants