google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

Add ability to yield interpreter #410

Closed cheezypoofs closed 1 year ago

cheezypoofs commented 2 years ago

I'd like to submit a PR for a way to yield the interpreter loop back to the code managing the interpreter. The use case is to allow the managing code to use something more sophisticated than maxSteps in the thread context to either pause or cancel execution. Specifically, this can be useful for throttling execution of instructions in constrained environments.

maxSteps will cancel execution when reached and appears not to allow resumption of work. Also, it's possible the code managing the interpreter will want to use true resource utilization of the process or simply elapsed time to impose a stall to achieve a particular duty cycle.

My proposal would be to add a similar counter to maxSteps like yieldSteps and an optional Yield (open to a better name) callback on the thread similar to Print and Load. The non-nil presence of this callback and a non-zero yieldSteps will trigger the callback.

adonovan commented 2 years ago

If your goal is to constrain resource usage, I'm skeptical this can be reliably achieved within the process: a Starlark program can easily chew up vast amounts of memory even if you constrain its CPU usage or step count, and there's no way to account for that. I would suggest you investigate system-level resource isolation mechanisms like cgroups (containers). This may require you to use more processes in your design.

Do you know of any other embedded interpreter that has successfully used the proposed mechanism?

cheezypoofs commented 2 years ago

Appreciate the quick response. I totally understand the skepticism. From the perspective of absolute use of exclusive resources like memory, you are correct. This approach would have no impact (and in fact could lead to slightly longer use of these resources as the execution was slowed down). I can attest that I've worked on a few projects, sadly none of them open source, that use this approach with quite good success. The suppression of utilization is not necessarily tied to just CPU, but can be tied to other usages too. The approach is both naive and simplistic, but in practice rather effective and achieving a desired duty cycle suppression of rate-based resource usage like cpu and i/o.

So, I can't point you at existing interpreters but can hopefully anecdotally attest to the effectiveness of such an approach when coupled with process (or even per-thread) resource sampling and a heuristic that applies an adaptive approach to the previous samples to adjust current and future executions. For example, when you hope to achieve a utilization of 5%, a simple duty cycle of 1/20 doesn't necessarily work perfectly, but coupling that with a sampling algo that answers "how close did we achieve the target from the last duty cycle?" and then adjusting, and using windows of 50-250ms works quite well.

In any case, perhaps there is a better approach? Perhaps such a thing can be used for more than just this purpose. I notice that there is a private member for doing profiling and I wonder if we could override the profiling mechanism to do a custom function-call interceptor that could roughly achieve a similar goal while also allowing someone to specialize how profiling is done?

cheezypoofs commented 1 year ago

For reference (what i would consider to PR for this if it is accepted): https://github.com/cheezypoofs/starlark-go/commit/498c8e8579c84fbdf2839fb1d862e53d91d002b0

cheezypoofs commented 1 year ago

(note: small discussion happening in the linked branch)

cheezypoofs commented 1 year ago

Based on the discussion above, considering this as a cleaner way to achieve the same effect: https://github.com/cheezypoofs/starlark-go/commit/6064dbd3ab26c9f27582d00fbde27821cb7afce3

cheezypoofs commented 1 year ago

Opened #411 for this issue.

adonovan commented 1 year ago

Now that #411 is submitted, I'm going to consider this closed.