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

Delegate registration of FieldMetrics to blocking threadpool #2014

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Nov 19, 2023

This is something that's been bugging me for some time. We've seen previously that writing the field metrics to the registry can be relatively expensive when it comes to CPU time.
To reduce runtime performance overhead, we've been forking the process. However, the forked fiber still runs on the default executor which has very limited number of threads (sized to the number of available CPU). In environments with low number of CPU, this means that we might be "starving" the default executor threadpool for writing field metrics.

One option (as in this PR) is to run the metrics registering effect on the blocking executor. However, this has the drawback of spawning additional threads in the unbounded blocking threadpool.

Alternatives:

  1. Allow the user to explicitly define the executor. Downside is that it's very unlikely users will bother to change it
  2. Create a dedicated single-thread executor for writing metrics to the registry. Downside of this approach is that there is a very small chance this cause memory leaks in environment with large number of CPUs processing requests.
  3. Allow users to explicitly provide an executor, and default to a single-thread executor if unspecified. Only downside to this is that it might be too big of a hammer if we think that using ZIO.blocking is good enough or if (2) is not an issue

Thoughts?

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@kyri-petrou kyri-petrou merged commit 2646d9a into series/2.x Nov 19, 2023
10 checks passed
@kyri-petrou kyri-petrou deleted the field-metrics-optimizations branch November 19, 2023 06:58
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