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

Speed up user initialization #557

Conversation

Michael-Hollister
Copy link
Contributor

When running goose tests with large amounts of goose users on my machine, it takes roughly 1 minute to initialize 1000 goose users. It appears that building each reqwest client for each goose user is an expensive operation. If we change the behavior to build only 1 reqwest client, and clone the object when creating the goose users, we can significantly increase initialization performance. With this change, I can create a few million goose users and only takes a couple of seconds to complete the initialization phase on my machine.

@jeremyandrews
Copy link
Member

Wow, that’s indeed an impressive optimization. At a first glance and with minimal testing it looks great. I’ll look a bit closer shortly, and likely will merge as is. Thanks!

@jeremyandrews
Copy link
Member

I've done a bit more testing.

On Linux, this is a massive improvement. Without the patch, trying to start too many users not only takes a very long time, but it uses very large amounts of memory. With the patch, it launches very quickly.

On Mac OS X (Silicon) there's no big difference with or without the patch. I'm still trying to understand why, but even if it's a Linux-only improvement it's still a nice improvement.

@Michael-Hollister
Copy link
Contributor Author

Michael-Hollister commented Aug 22, 2023

Ah, that is interesting. Yeah, I am primarily using Linux for running my tests. I've also tested this on Windows, and I observe similar behavior you see on Mac OS X, where user initialization is very quick without the patch and not taking up a significant amount of memory.

I also tried using the RustLS TLS bindings on Linux (https://book.goose.rs/config/rustls.html#rustls), but seems like that had no effect with the un-patched goose version in regards to speed/memory usage.

Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

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

This is a significant win on starting a load test on Linux. It doesn't impact the speed of starting a load test on Mac OS X, but it does consistently improve the reliability of large load tests. I hope to keep digging into why, but as there's no regressions and only improvements happy to commit as is.

@jeremyandrews jeremyandrews merged commit 136dbaf into tag1consulting:main Aug 22, 2023
2 checks passed
jeremyandrews added a commit that referenced this pull request Aug 22, 2023
@jeremyandrews
Copy link
Member

Thanks for the clean patch and impressive optimization contribution!

@jeremyandrews jeremyandrews mentioned this pull request Aug 28, 2023
jeremyandrews added a commit to jeremyandrews/goose that referenced this pull request Dec 20, 2023
@jeremyandrews
Copy link
Member

jeremyandrews commented Dec 20, 2023

Unfortunately this optimization has to be reverted. It results in the CookieJar and Headers to be shared between all clients: while this may not matter in some circumstances, it's not a good default. See this PR where I've added tests and reverted the .clone() optimization:
#575

I'd welcome a new PR to optionally restore this optiization if it wraps the optimization in a configuration option, so you'd have to intentionally disable cookie/header support for this quick startup.

@Michael-Hollister
Copy link
Contributor Author

Sure, I understand and thanks for letting me know. All of my Goose usage is done on Linux with large numbers of goose users, so this is essential for my use-case. I'll work on submitting a new PR making this a configuration option.

@jeremyandrews
Copy link
Member

@Michael-Hollister I took a stab at one possible solution, can you give it a try?
#578

I was originally thinking it could be a run-time option, but figured it was better to not even compile the related Reqwest code if it's not being used.

@Michael-Hollister
Copy link
Contributor Author

@Michael-Hollister I took a stab at one possible solution, can you give it a try? #578

I was originally thinking it could be a run-time option, but figured it was better to not even compile the related Reqwest code if it's not being used.

Thanks @jeremyandrews! Tested the branch and the cargo feature approach works fine for me.

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