starlark: support thread cancellation and limits on computation #298
Conversation
starlark/eval.go
Outdated
| // before and after a computation. It may also be used to limit | ||
| // a computation to k steps by setting its initial value to | ||
| // 1<<64 - k. | ||
| Steps uint64 |
There was a problem hiding this comment.
No change needed necessarily, just thinking about alternatives:
When I worked on V8, they had a counter which was set to some value, then decremented at function returns and loop back branches. If it reached zero, the profiler was notified, which could trigger optimization.
I think it's a little more intuitive to have a decrementing counter, since you can initially set it to k rather than 1<<64 - k. But then measuring how many steps were taken is harder, so perhaps there's no advantage either way.
In any case, I wonder if it would be better to provide methods to get / set this, rather than exporting the counter itself. There are a lot of uses and possible representations for a counter like this, and it could easily change in the future.
There was a problem hiding this comment.
Fair point about encapsulation; I've changed the API to a pair of functions ExecutionSteps and SetMaxExecutionSteps. (No set or reset operation is necessary.) The check must now do an extra load of 'maxSteps', but it's in the same cache line as 'steps'.
| if thread.Steps == 0 { | ||
| thread.Cancel("too many steps") | ||
| } | ||
| if reason := atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&thread.cancelReason))); reason != nil { |
There was a problem hiding this comment.
The atomic load, compare, and branch on every iteration seems expensive.
Cancel could set Steps to ^0, but I suppose an atomic load would still be needed.
That could be optimized by keeping a local step count and updating the thread's step count on certain instructions (on builtin, return, or back branch) or when the local step count crosses some constant threshold.
No need to optimize that now, but it may require changing the contract of Steps in the future. Better to keep it abstract or vague.
There was a problem hiding this comment.
I measured it on x86 and it wasn't noticeable (sorry I didn't keep the data). The address is always in L1 cache, the branch is extremely predictable, and on Intel at least, every load is an atomic load. (The atomic load is much cheaper than selecting on the Done channel of a context: even though fundamentally that too boils down to an atomic load, compare, and branch, the function call overheads are significant.)
I considered using a local variable but it complicates the logic, and I suspect it could increase register pressure in the rest of the bytecode interpreter.
starlark/eval.go
Outdated
| // Starlark execution, by computing the difference in its value | ||
| // before and after a computation. It may also be used to limit | ||
| // a computation to k steps by setting its initial value to | ||
| // 1<<64 - k. |
There was a problem hiding this comment.
Consider adding a note that the number of steps taken by a given computation is an implementation detail and may change.
alandonovan
force-pushed
the
steps-and-cancel
branch
from
e94f8a2
to
a121eb0
Compare
* vendor: update starlark * terminal: use new mechanism to cancel starlark threads See: google/starlark-go#298
…le#298) This change adds two related features: (1) asynchronous cancellation of Starlark threads (Thread.Cancel), and (2) recording and limiting of the number of abstract computation steps. Fixes issue google#252 Fixes issue google#160
…le#298) This change adds two related features: (1) asynchronous cancellation of Starlark threads (Thread.Cancel), and (2) recording and limiting of the number of abstract computation steps. Fixes issue google#252 Fixes issue google#160
* vendor: update starlark * terminal: use new mechanism to cancel starlark threads See: google/starlark-go#298
This change adds two related features:
(1) asynchronous cancellation of Starlark threads (Thread.Cancel), and
(2) recording and limiting of the number of abstract computation steps.
Fixes issue #252
Fixes issue #160