From d978b556e6d3189760e36fbc0fefe683618cf6ad Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 15 Oct 2022 16:10:10 -0700 Subject: feat(tvix/eval): deduplicate overlap between Closure and Thunk This commit deduplicates the Thunk-like functionality from Closure and unifies it with Thunk. Specifically, we now have one and only one way of breaking reference cycles in the Value-graph: Thunk. No other variant contains a RefCell. This should make it easier to reason about the behavior of the VM. InnerClosure and UpvaluesCarrier are no longer necessary. This refactoring allowed an improvement in code generation: `Rc>`s are now created only for closures which do not have self-references or deferred upvalues, instead of for all closures. OpClosure has been split into two separate opcodes: - OpClosure creates non-recursive closures with no deferred upvalues. The VM will not create an `Rc>` when executing this instruction. - OpThunkClosure is used for closures with self-references or deferred upvalues. The VM will create a Thunk when executing this opcode, but the Thunk will start out already in the `ThunkRepr::Evaluated` state, rather than in the `ThunkRepr::Suspeneded` state. To avoid confusion, OpThunk has been renamed OpThunkSuspended. Thanks to @sterni for suggesting that all this could be done without adding an additional variant to ThunkRepr. This does however mean that there will be mutating accesses to `ThunkRepr::Evaluated`, which was not previously the case. The field `is_finalised:bool` has been added to `Closure` to ensure that these mutating accesses are performed only on finalised Closures. Both the check and the field are present only if `#[cfg(debug_assertions)]`. Change-Id: I04131501029772f30e28da8281d864427685097f Signed-off-by: Adam Joseph Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019 Tested-by: BuildkiteCI Reviewed-by: tazjin --- tvix/eval/src/opcode.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) (limited to 'tvix/eval/src/opcode.rs') diff --git a/tvix/eval/src/opcode.rs b/tvix/eval/src/opcode.rs index 6b2783716c..382a857ef9 100644 --- a/tvix/eval/src/opcode.rs +++ b/tvix/eval/src/opcode.rs @@ -144,25 +144,27 @@ pub enum OpCode { OpCall, OpTailCall, OpGetUpvalue(UpvalueIdx), + // A Closure which has upvalues but no self-references OpClosure(ConstantIdx), - - // Thunks - OpThunk(ConstantIdx), + // A Closure which has self-references (direct or via upvalues) + OpThunkClosure(ConstantIdx), + // A suspended thunk, used to ensure laziness + OpThunkSuspended(ConstantIdx), OpForce, /// Finalise initialisation of the upvalues of the value in the /// given stack index after the scope is fully bound. OpFinalise(StackIdx), - // [`OpClosure`] and [`OpThunk`] have a variable number of - // arguments to the instruction, which is represented here by - // making their data part of the opcodes. Each of these two - // opcodes has a `ConstantIdx`, which must reference a - // `Value::Blueprint(Lambda)`. The `upvalue_count` field in - // that `Lambda` indicates the number of arguments it takes, and - // the `OpClosure` or `OpThunk` must be followed by exactly this - // number of `Data*` opcodes. The VM skips over these by - // advancing the instruction pointer. + // [`OpClosure`], [`OpThunkSuspended`], and [`OpThunkClosure`] have a + // variable number of arguments to the instruction, which is + // represented here by making their data part of the opcodes. + // Each of these two opcodes has a `ConstantIdx`, which must + // reference a `Value::Blueprint(Lambda)`. The `upvalue_count` + // field in that `Lambda` indicates the number of arguments it + // takes, and the opcode must be followed by exactly this number + // of `Data*` opcodes. The VM skips over these by advancing the + // instruction pointer. // // It is illegal for a `Data*` opcode to appear anywhere else. DataLocalIdx(StackIdx), -- cgit 1.4.1