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

improve the spawn feature #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

boboalex
Copy link

@boboalex boboalex commented Oct 23, 2022

Proposed changes

improve the spawn feature

  1. boomer receive the spawn message from locust, then calculate the concurrency difference among current step and previous step, then increase request goroutines or decrease request goroutines which is depended on the sign of difference value
  2. adjust the unit test with new spawn codes

Types of changes

What types of changes does your code introduce to boomer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

1. boomer receive the spawn message from locust, then calculate the concurrency difference among current step and previous step, then increase reqeust goroutines or decrease request goroutines which is depended on the sign of difference value
2. adjust the unit test with new spawn codes
@boboalex
Copy link
Author

boboalex commented Oct 23, 2022

self test plan

the self-test contains unit test and feature test based on the new codes, for the feature test, testing cases are as follows:
image

self test result

unit test

image

image

image

image

image

feature test

main process test

image

add new boomer instance test

image

quit one boomer instance test

image

load shape test

image

@@ -121,7 +121,10 @@ func (r *runner) outputOnStop() {
}

func (r *runner) spawnWorkers(spawnCount int, quit chan bool, spawnCompleteFunc func()) {
log.Println("Spawning", spawnCount, "clients immediately")
for i := 0; i > spawnCount; i-- {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a new stopWorkers function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a new stopWorkers function?

OK, so u think it might be a better way to define another function such as stopWorkers which could handle spawnCount < 0

// those goroutines will exit when r.safeRun returns
numClients := int(atomic.LoadInt32(&r.numClients))
for i := 0; i < numClients; i++ {
r.stopChan <- true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a buffered channel? So message processing won't block here and wait for task execution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a buffered channel? So message processing won't block here and wait for task execution.

so u might think the channel should be buffered if we send true signal in for loop avoiding blocking the message processing? so for this way, the channel capacity need to be initialized for each spawn, is that right~

Copy link
Author

@boboalex boboalex Oct 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a buffered channel? So message processing won't block here and wait for task execution.

so u might think the channel should be buffered if we send true signal in for loop avoiding blocking the message processing? so for this way, the channel capacity need to be initialized for each spawn, is that right~

i found it is not safe to resize the channel capacity for each spawn step, especially when the spawn count is decreasing comparing with previous spawn step. For instance, if spawn count is 30 of the previous step and then spawn count of current step is 20, if stopChan is resized for the current spawn period, then 10 of the goroutines could not exit normally.
therefore, I found another plan: try to make the stopChan as "unbounded channel": https://github.com/smallnest/chanx/blob/main/unbounded_chan.go then we could just focus on handling the signal sending to channel, the channel is buffered and also safe as i tested. in addition, i think the version 1.0.0 is enough for boomer to use.
however, it would change the type of stopchan and also influence some unit test cases

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a bounded channel is enough and you don't need to resize it.

See aslo
https://github.com/myzhan/boomer/blob/master/stats.go#L42

r.numClients = 0
// prevent receiving spawn message from master when boomer is handling stopping,
// that might happen when the numClients is too large and might take a while for stopping all request goroutines
for {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this loop if we use a buffered channel?

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.

2 participants