Expose resource based autotuning #293

Sushisource · 2024-06-28T00:05:24Z

What was changed

Expose resource based auto-tuner

Why?

Adding in all SDKs

Checklist

Closes
How was this tested:
Added some tests, manual verification with omes / existing core tests
Any docs updates needed?
Will be after experimental phase

Sushisource · 2024-06-28T00:06:00Z

src/Temporalio/Bridge/OptionsExtensions.cs

-            var nonDetWorkflowFailForTypes = options.Workflows.Where(
-                w => w.FailureExceptionTypes?.Any(
-                    t => t.IsAssignableFrom(typeof(WorkflowNondeterminismException))) ?? false).
-                Select(w => w.Name).ToArray();


This was unused

Thanks! Wonder why there isn't an unused-local-var check in .NET, maybe I have it turned off

cretz

Mostly LGTM. I do have a suggestion about reworking the worker tuner hierarchy in a comment below.

cretz · 2024-06-28T13:14:29Z

src/Temporalio/Bridge/include/temporal-sdk-bridge.h

+typedef enum SlotSupplier_Tag {
+  FixedSize,
+  ResourceBased,
+} SlotSupplier_Tag;
+
+typedef struct SlotSupplier {
+  SlotSupplier_Tag tag;
+  union {
+    struct {
+      struct FixedSizeSlotSupplier fixed_size;
+    };
+    struct {
+      struct ResourceBasedSlotSupplier resource_based;
+    };
+  };
+} SlotSupplier;


I think this makes our header file a bit uglier to consume from C compared to mutually exclusive options (technically doesn't matter though)

🤷‍♂️ Eh, not really a problem IMO. If we care about the header being as easy as possible we can do that when we have another consumer, but honestly I think this is pretty easy to use as-is.

cretz · 2024-06-28T13:15:18Z

src/Temporalio/Bridge/src/worker.rs

+
+#[repr(C)]
+#[derive(Clone, Copy, PartialEq, Debug)]
+pub struct ResourceBasedTunerConfig {


Suggested change

pub struct ResourceBasedTunerConfig {

pub struct ResourceBasedTunerOptions {

cretz · 2024-06-28T13:17:07Z

src/Temporalio/Bridge/src/worker.rs

@@ -49,6 +48,43 @@ pub struct WorkerOptions {
    nondeterminism_as_workflow_fail_for_types: ByteArrayRefArray,
 }

+#[repr(C)]
+pub struct TunerHolder {


Suggested change

pub struct TunerHolder {

pub struct WorkerTuner {

I think this is a better name, but doesn't matter much to have this separate/different "holder" terminology in the bridge only I suppose so long as user's don't have to see it

This matches the thing it's getting turned into Core side so I think it's worth keeping

cretz · 2024-06-28T13:22:10Z

src/Temporalio/Worker/TemporalWorkerOptions.cs

        /// </summary>
-        public int MaxConcurrentActivities { get; set; } = 100;
+        public int? MaxConcurrentActivities { get; set; }


This is technically a backwards incompatible change (people could use this property in some log or something). I wonder if it'd be better to leave these and just say this is ignored if Tuner is non-null? I don't have a super strong opinion here, we can probably break this kind of compat if we must.

I think it's probably worth it. It's more clear at the type level about what's going on and I think is fairly unlikely to break anyone and easy to fix if it is.

cretz · 2024-06-28T13:28:48Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+        /// Gets a slot supplier for workflow tasks.
+        /// </summary>
+        /// <returns>A slot supplier for workflow tasks.</returns>
+        public abstract ISlotSupplier GetWorkflowTaskSlotSupplier();


Suggested change

public abstract ISlotSupplier GetWorkflowTaskSlotSupplier();

public abstract ISlotSupplier WorkflowTaskSlotSupplier { get; }

Lets make these abstract properties instead of getters

cretz · 2024-06-28T13:34:09Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+    /// <summary>
+    /// WorkerTuners allow for the dynamic customization of some aspects of worker configuration.
+    /// </summary>
+    public abstract class WorkerTuner


I have an alternative suggestion here. .NET prefers interfaces and the default worker tuner can just be composite, so how about something like:

public interface IWorkerTuner { ISlotSupplier WorkflowTaskSlotSupplier { get; } ISlotSupplier ActivityTaskSlotSupplier { get; } ISlotSupplier LocalActivitySlotSupplier { get; } } // Not a record on purpose public class WorkerTuner : IWorkerTuner { public static WorkerTuner CreateResourceBased(...) public static WorkerTuner CreateFixed(...) public WorkerTuner( ISlotSupplier workflowTaskSlotSupplier, ISlotSupplier activityTaskSlotSupplier, ISlotSupplier localActivitySlotSupplier) { WorkflowTaskSlotSupplier = workerTaskSlotSupplier; ActivityTaskSlotSupplier = activityTaskSlotSupplier; LocalActivitySlotSupplier = localActivitySlotSupplier; } public ISlotSupplier WorkflowTaskSlotSupplier { get; init; } public ISlotSupplier ActivityTaskSlotSupplier { get; init; } public ISlotSupplier LocalActivitySlotSupplier { get; init; } }

And get rid of CompositeTuner and accept IWorkerTuner in the worker options. I think this is more idiomatic .NET (have interface and common impl as the primary impl). Arguably it'd be more idiomatic in .NET (unlike Python and others) to also make ResourceBasedTuner a class with a constructor instead of a static helper here. So could be something like:

public interface IWorkerTuner { ISlotSupplier WorkflowTaskSlotSupplier { get; } ISlotSupplier ActivityTaskSlotSupplier { get; } ISlotSupplier LocalActivitySlotSupplier { get; } } // Not a record on purpose public class WorkerTuner : IWorkerTuner { public WorkerTuner( ISlotSupplier workflowTaskSlotSupplier, ISlotSupplier activityTaskSlotSupplier, ISlotSupplier localActivitySlotSupplier) { WorkflowTaskSlotSupplier = workerTaskSlotSupplier; ActivityTaskSlotSupplier = activityTaskSlotSupplier; LocalActivitySlotSupplier = localActivitySlotSupplier; } public ISlotSupplier WorkflowTaskSlotSupplier { get; init; } public ISlotSupplier ActivityTaskSlotSupplier { get; init; } public ISlotSupplier LocalActivitySlotSupplier { get; init; } } public class ResourceBasedTuner : WorkerTuner { public ResourceBasedTuner( double targetMemoryUsage, double targetCpuUsage, ResourceBasedSlotSupplierOptions? workflowOptions = null, ResourceBasedSlotSupplierOptions? activityOptions = null, ResourceBasedSlotSupplierOptions? localActivityOptions = null) : base( new ResourceBasedSlotSupplier(workflowOptions ?? new(), new(targetMemoryUsage, targetCpuUsage), new ResourceBasedSlotSupplier(activityOptions ?? new(), new(targetMemoryUsage, targetCpuUsage), new ResourceBasedSlotSupplier(localActivityOptions ?? new(), new(targetMemoryUsage, targetCpuUsage)) { } } public class FixedSizeTuner : WorkerTuner { public FixedSizeTuner(int workflowTaskSlots, int activityTaskSlots, int localActivitySlots) : base( new FixedSizeSlotSupplier(workflowTaskSlots), new FixedSizeSlotSupplier(activityTaskSlots), new FixedSizeSlotSupplier(localActivitySlots)) { } }

I think that last one is the most idiomatic

cretz · 2024-06-28T13:39:17Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+        /// <param name="numActivityTaskSlots">The number of available activity task slots.</param>
+        /// <param name="numLocalActivitySlots">The number of available local activity slots.</param>
+        /// <returns>The tuner.</returns>
+        public static WorkerTuner CreateFixed(


Suggested change

public static WorkerTuner CreateFixed(

public static WorkerTuner CreateFixedSize(

To be consistent

cretz · 2024-06-28T13:42:07Z

src/Temporalio/Bridge/OptionsExtensions.cs

+                this Temporalio.Worker.Tuning.ISlotSupplier supplier,
+                bool isWorkflow)


Suggested change

this Temporalio.Worker.Tuning.ISlotSupplier supplier,

bool isWorkflow)

this Temporalio.Worker.Tuning.ISlotSupplier supplier,

bool isWorkflow)

Inconsistent spacing

Dotnet format seems to just... not format stuff I would expect it to fairly often

Yeah, I don't use it to rewrite code, I just use it to know CI will pass

cretz · 2024-06-28T13:44:22Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+            uint numWorkflowTaskSlots,
+            uint numActivityTaskSlots,
+            uint numLocalActivitySlots)


While it makes more sense in Rust, it's not that common in .NET to use uints unless you really need to (note our options don't really). Can check for negative at runtime if needed. Same type of thing as Go, most people pass around the common integer type and usually they aren't expected to convert for value-safety reasons.

I'm not sure what would rise to the definition of "really need to" in the sense that things that shouldn't be negative shouldn't be negative and that's a fairly binary distinction.

It's annoying as hell that unsigned types exist in these languages but aren't used for whatever reason. Oh well though, I'll change it.

Yeah, that is a bit annoying. I think the reasoning is more about the specific-bit-size integers and just carried over to here. For those, people avoid them unless they need specific binary sizes or are in a space-conscious place (e.g. embedded). And I think the general "use int for integers unless there is a good use case not to" was kinda the general accepted thing and applied to unsigned as well as specific-bit-size types. The only reason I bring up here is because in cases where uints or other things are used, .NET has a whole thing about unchecked conversions and it's a bit of a pain for devs using ints everywhere else.

cretz · 2024-06-28T13:59:11Z

src/Temporalio/Worker/Tuning/FixedSizeSlotSupplier.cs

+    /// A slot supplier that will only ever issue at most a fixed number of slots.
+    /// </summary>
+    /// <param name="NumSlots">The maximum number of slots that will ever be issued.</param>
+    public sealed record FixedSizeSlotSupplier(uint NumSlots) : ISlotSupplier;


Suggested change

public sealed record FixedSizeSlotSupplier(uint NumSlots) : ISlotSupplier;

public sealed record FixedSizeSlotSupplier(int SlotCount) : ISlotSupplier;

Besides the int/uint thing, I think .NET would prefer either Count suffix or just Slots over "num".

cretz · 2024-06-28T20:22:32Z

src/Temporalio/Worker/Tuning/ResourceBasedSlotSupplierOptions.cs

+        uint? MinimumSlots = null,
+        uint? MaximumSlots = null,


Can we switch these to ints too?

cretz

May need to downgrade proto to 23.x and regen those

cretz · 2024-06-28T20:25:55Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+            int numWorkflowTaskSlots,
+            int numActivityTaskSlots,
+            int numLocalActivitySlots)


Suggested change

int numWorkflowTaskSlots,

int numActivityTaskSlots,

int numLocalActivitySlots)

int workflowTaskSlots,

int activityTaskSlots,

int localActivitySlots)

Not a big deal, but if you do end up making another commit, might as well (or suffix with Count or whatever). Matters a bit more here than in Java because .NET supports named parameters.

cretz · 2024-06-28T20:27:07Z

src/Temporalio/Worker/Tuning/WorkerTuner.cs

+            int numWorkflowTaskSlots,
+            int numActivityTaskSlots,
+            int numLocalActivitySlots)


Btw, for single-line method implementations, can use => instead of a block + return. Makes no difference though.

…hanges

cretz

LGTM, will leave unapproved pending further investigation for the worker.is_null() thing on worker_free and there may be some whitespace issues.

Sushisource and others added 8 commits June 26, 2024 16:17

Initial plumbing for most of the options

0cb14bf

Update Core

4c630d7

Bridge partial work

db8ef87

Generate CS from C header

2a1858c

Type conversions for bridge

5f7c19b

Mutual exclusion checking / defaults

3fbc3bd

Add tests

6594c89

Some final cleanup / todo addressing

a192c53

Sushisource commented Jun 28, 2024

View reviewed changes

Sushisource added 2 commits June 27, 2024 17:06

Strip leftover dbg

7805685

Missing docstring

e6a3ba5

cretz reviewed Jun 28, 2024

View reviewed changes

Sushisource added 4 commits June 28, 2024 10:25

Regen protos

ae67460

Rename tunerconfig->tuneroptions

c5bfd55

Smaller review fixes

82815f3

Change worker tuner to interface / remove composite

f49476e

cretz reviewed Jun 28, 2024

View reviewed changes

cretz approved these changes Jun 28, 2024

View reviewed changes

Sushisource added 3 commits June 28, 2024 15:02

Wake up github

1a39504

More proto gobbledygook

fb69ebb

Add upload of git diff to make it easier to deal with proto/pinvoke c…

97bde30

…hanges

Sushisource force-pushed the expose-autotune branch 3 times, most recently from 6e7d4cd to fabd19b Compare June 28, 2024 23:26

Hopefully last round of proto nonsense

01f3db6

Sushisource force-pushed the expose-autotune branch from fabd19b to 01f3db6 Compare June 29, 2024 00:00

Sushisource added 2 commits June 28, 2024 17:26

Address remaining uint

4941fcc

Unclear on why this is needed suddenly

65ac9ad

cretz reviewed Jul 1, 2024

View reviewed changes

Sushisource added 3 commits July 1, 2024 10:24

Merge branch 'main' into expose-autotune

6e6bc89

Add correct IsInvalid checks for unmanaged code

9a21c83

Fix whitespace nonsense

eecbedd

Sushisource merged commit 29753e5 into main Jul 1, 2024
7 checks passed

Sushisource deleted the expose-autotune branch July 1, 2024 22:18

	pub struct ResourceBasedTunerConfig {
	pub struct ResourceBasedTunerOptions {

	public abstract ISlotSupplier GetWorkflowTaskSlotSupplier();
	public abstract ISlotSupplier WorkflowTaskSlotSupplier { get; }

	public static WorkerTuner CreateFixed(
	public static WorkerTuner CreateFixedSize(

		this Temporalio.Worker.Tuning.ISlotSupplier supplier,
		bool isWorkflow)

	public sealed record FixedSizeSlotSupplier(uint NumSlots) : ISlotSupplier;
	public sealed record FixedSizeSlotSupplier(int SlotCount) : ISlotSupplier;

Expose resource based autotuning #293

Expose resource based autotuning #293

Uh oh!

Conversation

Sushisource commented Jun 28, 2024

What was changed

Why?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cretz Jun 28, 2024 •

edited

Loading

cretz Jun 28, 2024 •

edited

Loading

cretz Jun 28, 2024 •

edited

Loading