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

Optimize / fix ScenarioConfigs.GetFullExecutionRequirements() #2576

Open
na-- opened this issue Jun 20, 2022 · 0 comments
Open

Optimize / fix ScenarioConfigs.GetFullExecutionRequirements() #2576

na-- opened this issue Jun 20, 2022 · 0 comments
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 performance refactor

Comments

@na--
Copy link
Member

na-- commented Jun 20, 2022

While reviewing #2573, I noticed some strange code in ScenarioConfigs.GetFullExecutionRequirements(), specifically this part:

k6/lib/executors.go

Lines 318 to 321 in 556747f

currentTimeOffset = step.TimeOffset
currentPlannedVUs[step.configID] = step.PlannedVUs
currentMaxUnplannedVUs[step.configID] = step.MaxUnplannedVUs
addCurrentStepIfDifferent()

It seems that currentTimeOffset is meaningless and we will always add all intermediate steps to consolidatedSteps, which is not ideal. It's probably just a minor performance problem and not a consequential bug because GetFullExecutionRequirements() is not used for anything besides figuring out the max test VUs and duration at this point in time, but it could be a nasty surprise for us in the future if left unfixed.

Digging into the history of the code, it seems like it's like this because of a bugfix (#1362) for another issue (#1358) from before we even release executors in k6 v0.27.0 🤯 Before that the code made a bit more sense, even if it was buggy:

k6/lib/executors.go

Lines 311 to 316 in 55a49d5

for _, step := range trackedSteps {
// If the time offset is different, create a new step with the current values
if step.TimeOffset != currentTimeOffset {
addCurrentStepIfDifferent()
currentTimeOffset = step.TimeOffset
}

And I think the sorting change from #1362 and something like the if change from #2573 might also solve the original issue without having all of the confusing intermediate steps in the end result 🤔 And if we can't (which I doubt), we should at least better document the current behavior, so that the expectations of its users aren't broken.

@na-- na-- added bug performance refactor evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Jun 20, 2022
@na-- na-- added this to the v0.40.0 milestone Jun 20, 2022
@mstoykov mstoykov modified the milestones: v0.40.0, v0.41.0 Aug 30, 2022
@na-- na-- modified the milestones: v0.41.0, TBD Oct 28, 2022
@codebien codebien removed this from the TBD milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 performance refactor
Projects
None yet
Development

No branches or pull requests

3 participants