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

How to set workerPool.MaxIdleWorkerDuration? #1178

Closed
kilosliu opened this issue Dec 18, 2021 · 12 comments
Closed

How to set workerPool.MaxIdleWorkerDuration? #1178

kilosliu opened this issue Dec 18, 2021 · 12 comments

Comments

@kilosliu
Copy link
Contributor

I have found usage of workerPool.MaxIdleWorkerDuration in GoLand, but only find it was used in workerPool.getMaxIdleWorkerDuration(), and there is no other way to set workerPool.MaxIdleWorkerDuration, so workerPool.getMaxIdleWorkerDuration() always returns 10 * time.Second. How could I set the workerPool.MaxIdleWorkerDuration to a bigger value so that the worker goroutine can live longer?

func (wp *workerPool) getMaxIdleWorkerDuration() time.Duration {
	if wp.MaxIdleWorkerDuration <= 0 {
		return 10 * time.Second
	}
	return wp.MaxIdleWorkerDuration
}
@erikdubbelboer
Copy link
Collaborator

You're right, it's not possible. What would be your use case? Would you like to set it lower or higher and for what reason?

@kilosliu
Copy link
Contributor Author

The purpose of workerPool is to reduce the overhead caused by the frequent creation and destruction of goroutine. If worker goroutine expiration time can be set, users can determine how long a goroutine expires based on their own usage scenarios. For example, if there is a rush of traffic every ten seconds or a minute, the workerPool will lose its meaning because the goroutines created in the previous wave are destroyed before the next wave arrives (although fasthttp still outperforms std http lib in this case). In this case, I would probably prefer to set the expiration time of the worker goroutine slightly longer to reduce the cost of creating and destroying the goroutine.

@erikdubbelboer
Copy link
Collaborator

Have you tried making a change to fasthttp to increase the timeout? Since goroutine creating is very cheap in Go I wonder if it would have any measurably effect. If you can show there is a measurable effect I can expose the API so you can change it.

@kilosliu
Copy link
Contributor Author

I'm sorry I couldn't figure out how to test the difference between unrestricted creation and destruction of goroutine and changing the expiration time of workerPool, I was inspired by panjf2000/ants and thought it might be good to allow the user to set the expiration time of worker goroutine.😢 Sorry again.

@erikdubbelboer
Copy link
Collaborator

No need to apologize. I have pushed a test branch which adds Server.MaxIdleWorkerDuration.

You can use the branch by running

go mod edit -replace github.com/valyala/fasthttp=github.com/valyala/fasthttp@a396af6384f8f09c9b052097528040189cdf326b
go mod tidy

(and go mod vendor depending if you vendor your dependencies or not).

After that you should be able to set Server.MaxIdleWorkerDuration and test the performance impact. Please let me know the result.

@kilosliu
Copy link
Contributor Author

Hi, thank you for your branch code, I didn't use fasthttp directly in my project, I'm using gofiber/fiber which is based on fasthttp, and after I replace the dependency of fasthttp, I have run 5 times of stress test on my API endpoint, there are the codes of stress test:

func main() {
	reg := regexp.MustCompile(`Requests/sec: + \d+.\d+`)
	const times = 12
	// const times = 50
	var res [times]float64
	for i := 0; i < times; i++ {
		cmd := exec.Command("zsh", "-c", "./codes/wrk.sh")
		out, err := cmd.Output()
		if err != nil {
			log.Fatal(string(out), err)
		}
		fmt.Println(string(out))
		num := string(reg.Find(out))
		req := strings.ReplaceAll(num[13:], " ", "")
		res[i], _ = strconv.ParseFloat(req, 64)
		if i < times-1 {
			time.Sleep(time.Second * 10) // Sleep to let the workerPool clean the goroutines
		}
	}
	sort.Float64s(res[:])
	var totalSpeed float64
	for i := 1; i < times-1; i++ {
		totalSpeed += res[i]
	}
	fmt.Printf("Max TPS: %.2f\n", res[times-1])
	fmt.Printf("Min TPS: %.2f\n", res[0])
	fmt.Printf("Avg TPS: %.2f\n", totalSpeed/(times-2))
}

It will use wrk to test the endpoint in 12 times, and remove the highest and lowest the result, then calculate the average of the result, the results are:

Round MaxIdleWorkerDuation = 5 * time.Second MaxIdleWorkerDuration = 3600 * time.Second
1 23673.77 25231.32
2 26728.02 28023.61
3 26980.24 26988.28
4 26944.42 26795.20
5 25307.70 26543.90
This may not be a rigorous result because it's not a direct test of fasthttp.

@kilosliu
Copy link
Contributor Author

The value in the table is the Avg TPS(Transaction/Request per second) on my HTTP endpoint, sorry.

@erikdubbelboer
Copy link
Collaborator

There is too much variance in those results. What I'm wondering now is if we should completely get rid of the worker pool instead. When I have time I'm going to run some benchmarks on that instead.

@kilosliu
Copy link
Contributor Author

Due to the system limit, I could only run the main func once at the same time, the 10 results are run in the following order:
3600, 5, 3600, 5, 3600, 5, 3600, 5, 3600, 5, so maybe it's not very accurate

@erikdubbelboer
Copy link
Collaborator

Removing the worker pool completely is slower. So I guess we should add MaxIdleWorkerDuration to Server. Do you want to make a pull request for this?

@kilosliu
Copy link
Contributor Author

I'd be happy to do that

@erikdubbelboer
Copy link
Collaborator

Fixed by #1183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants