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

feat(worker): Add support for concurrent pollers #1132

Merged
merged 16 commits into from
Jun 21, 2023

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Jun 5, 2023

What changed

  • Added WorkerOptions properties maxConcurrentWorkflowTaskPolls and maxConcurrentActivityTaskPolls, which allow controlling the number of pollers to be used to fetch Workflow/Activity tasks from the Task Queue. Properly adjusting these values should allow better filling of the corresponding execution slots.
  • Remove the @experimental tag on reuseV8Context
  • Better auto-configuration of maxCachedWorkflows, including using a different formula when reuseV8Context is enabled (fix [Bug] Default WorkerOptions.maxCachedWorkflows is too low #838)
  • Default value for maxConcurrentWorkflowTaskExecutions has been reduced to 40 (was previously 100), as higher values increase the risk of Workflow Task Timeouts unless other options are also tuned. This was not problem previously because the single poller was unlikely to fill in all execution slot anyway, so max would rarely reached).
  • Load balance create workflows requests amongst WF threads (fix [Feature Request] Better balancing of workflows between worker threads #608)
  • Better documentation of some performance/scaling-related properties in WorkerOptions.
  • Add more options to internal performance test runners, including specifying mTLS options (for testing against Temporal Cloud, pollers and WF thread pool size)
  • Updated Core to 782282e7

@mjameswh mjameswh requested a review from a team as a code owner June 5, 2023 20:16
@mjameswh
Copy link
Contributor Author

@bergundy Some major changes from the previous pass. Please take another look..

@Sushisource Would you mind taking a second look at changes in the worker-options.ts file? Any runtime constraint I forgot to check?

Also, in tests against Temporal Cloud, I had to set both WF and Act pollers to ~60 each to get peak performances for a single thread Worker running on my laptop. That seems way too high to use as a default value, but performance with only 2 pollers (ie. the minimum) were very disappointing. I settled on 10 pollers WF/Act as the default value. Comments?

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

I think the option defaults make sense 👍

Just some wording fixes in the docstrings.

packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
Co-authored-by: Spencer Judge <sjudge@hey.com>
@mjameswh
Copy link
Contributor Author

I think the option defaults make sense 👍

Just some wording fixes in the docstrings.

Thanks a lot! I need to find some good English spell/grammar checker that knows how to deal with source code, but hopefully, in the mean time, there is peer reviews 😆

packages/test/src/workflows/log-sink-tester.ts Outdated Show resolved Hide resolved
packages/testing/src/utils.ts Show resolved Hide resolved
packages/worker/src/runtime.ts Show resolved Hide resolved
packages/worker/src/runtime.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
packages/worker/src/worker-options.ts Show resolved Hide resolved
Comment on lines 247 to 248
* Setting this value higher than needed will generally not have negative impact on this Worker's performance; your
* server may however impose a limit on the total number of concurrent Workflow Task pollers.
Copy link
Member

Choose a reason for hiding this comment

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

But it may have a negative impact on the cluster's perf and may result in workflow tasks timing out if they're not processed in a timely fashion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could see in perf tests, task timeouts were linked to setting maxConcurrentWorkflowTaskExecutions too high, not to increasing maxConcurrentWorkflowTaskPolls. Core won't poll anyway if there is no execution slot available.

Still, I will change the phrasing to mention that too high might be negative on the server cluster.

packages/worker/src/worker-options.ts Show resolved Hide resolved
packages/worker/src/worker-options.ts Outdated Show resolved Hide resolved
@mjameswh mjameswh requested a review from bergundy June 21, 2023 18:39
* `maxConcurrentWorkflowTaskPolls` of `60`. Your millage may vary.
* In some performance test against Temporal Cloud, running with a single Workflow thread and the Reuse V8 Context
* option enabled, we reached peak performance with a `maxConcurrentWorkflowTaskExecutions` of `120`, and
* `maxConcurrentWorkflowTaskPolls` of `60` (worker machine: Apple M2 Max; ping of 74 ms to Temporal Cloud;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what the CPU load was and number of wf/s (not that we need to document that).

@bergundy
Copy link
Member

Awesome work on this!

@mjameswh mjameswh merged commit 737eb92 into temporalio:main Jun 21, 2023
24 checks passed
@mjameswh mjameswh deleted the core-concurrent-pollers branch June 21, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants