-
Notifications
You must be signed in to change notification settings - Fork 84
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
More efficient worker check-ins #4024
Conversation
This is amazing, @epicfaace !! |
self.terminate = True | ||
break | ||
self.process_runs() | ||
time.sleep(0.003) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this sleep and why just 0.003
specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure if we need it. I added it just in case so it doesn't call process_runs
too often.
I used 0.003 as we use sleep(0.003) elsewhere in the codebase when we don't need to sleep for too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just get rid of the sleep then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this cause 100% CPU utilization? 0.003 is very different from zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I re-added it.
We can always get this in with sleep(0.003)
(which is a strict improvement) and see later if there is space for more optimization.
time.sleep(self.checkin_frequency_seconds) | ||
last_checkin = time.time() | ||
# Process runs until it's time for the next checkin. | ||
while not self.terminate and ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. If we use threading.Timer, we won't block, that's correct, but then we'll be running process_runs
only once very 5 seconds. What makes bundles much faster is that we need to run process_runs
much more often -- not rate-limit it as much as self.checkin
is rate-limited.
Reasons for making this change
Fixes #4023 and fixes #3645. Running simple bundles is now blazing fast with minimal overhead! This changed the time on my local machine from staging a bundle -> finishing from 52 seconds to 5 seconds. This PR makes it faster in the following ways:
Logs for
cl run echo
with this PR:User logs:
Bundle manager logs:
Worker logs:
Logs for
cl run echo
without this PR:User logs:
Bundle manager logs:
Worker logs: