Optimize thread usage in monitor #1400
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
byucesoy
commented
Mar 27, 2024
byucesoy
force-pushed
the
limit-monitor-thread-count
branch
2 times, most recently
from
cf0bee5
to
ab2df92
Compare
|
Note: will read this more closely, because it is somewhat close to a rewrite, even in whitespace insensitive diff rendering mode. There's logical similarity, but I think I have to swap in the two designs from first principles, which is going to slow me down a bit. |
fdr
reviewed
Mar 29, 2024
byucesoy
force-pushed
the
limit-monitor-thread-count
branch
from
ab2df92
to
897bf6b
Compare
Yep, there are some parts that reuses the previous logic but yep, overall this is in the rewrite territory. |
fdr
reviewed
Apr 1, 2024
|
Replied to the questions, I'll add some comments. |
byucesoy
force-pushed
the
limit-monitor-thread-count
branch
from
897bf6b
to
7d90b48
Compare
enescakir
reviewed
Apr 19, 2024
byucesoy
force-pushed
the
limit-monitor-thread-count
branch
4 times, most recently
from
6605791
to
e92104b
Compare
fdr
approved these changes
Apr 24, 2024
fdr
reviewed
Apr 24, 2024
In monitor, we create 2 threads per resource; one for SSH event loop processing and one for actual pulse check. In previous version, each resource would keep their threads even after the pulse check is completed. This means the number of resources we can monitor at the same time is limited by the number of threads we can create. This commit changes the behavior so that after the pulse check is completed, the threads are released. This way, we can monitor significantly more resources at the same time. One drawback of the new approach is that we need to re-create the threads for each check. In my system creating 1000 threads takes about 0.025 seconds, so overhead is seems negligible. I also added a new helper method, needs_event_loop_for_pulse_check? to models. We actually don't need event loop for pulse check for most of the resources, only PostgresServer and MinioServer need it. Other resources rely on exec! to perform their pulse check which doesn't need event loop. In fact, I observed that extra event loop processing actually slows down the exec! calls. By taking this into consideration, we reduce the number of threads we create and also improve the speed of some pulse checks. Another change we are making is that removing the monitoring_interval from the model and hardcoding it in the monitor as 5 seconds. This removes capability of setting different monitoring intervals for different resources. Supporting this would require some work and since it is not used in the current implementation I decided to remove all together. If we need to support this in the future, we can add it back with some effort.
byucesoy
force-pushed
the
limit-monitor-thread-count
branch
from
b6f8215
to
1fb7765
Compare
enescakir
approved these changes
May 2, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In monitor, we create 2 threads per resource; one for SSH event loop processing
and one for actual pulse check. In previous version, each resource would keep
their threads even after the pulse check is completed. This means the number of
resources we can monitor at the same time is limited by the number of threads
we can create.
This commit changes the behavior so that after the pulse check is completed,
the threads are released. This way, we can monitor significantly more resources
at the same time.
One drawback of the new approach is that we need to re-create the threads for
each check. In my system creating 1000 threads takes about 0.025 seconds, so
overhead is seems negligible.
I also added a new helper method, needs_event_loop_for_pulse_check? to models.
We actually don't need event loop for pulse check for most of the resources,
only PostgresServer and MinioServer need it. Other resources rely on exec! to
perform their pulse check which doesn't need event loop. In fact, I observed
that extra event loop processing actually slows down the exec! calls. By taking
this into consideration, we reduce the number of threads we create and also
improve the speed of some pulse checks.
Another change we are making is that removing the monitoring_interval from the
model and hardcoding it in the monitor as 5 seconds. This removes capability of
setting different monitoring intervals for different resources. Supporting this
would require some work and since it is not used in the current implementation
I decided to remove all together. If we need to support this in the future, we
can add it back with some effort.