From 3419f63575fdf9c08477967183a0c34ff9433884 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Thu, 2 Feb 2023 01:29:50 +0300 Subject: fix(tvix/eval): ensure all evaluated thunks are correctly memoized This fixes a very complicated bug (b/246). Evaluation progresses *much* further after this, leading to several less complicated bugs likely being uncovered by this What was the problem? ===================== Previously, when evaluating a thunk, we had a code path that looked like this: match *thunk { ThunkRepr::Evaluated(Value::Thunk(ref inner_thunk)) => { let inner_repr = inner_thunk.0.borrow().clone(); drop(thunk); self.0.replace(inner_repr); } /* ... */ } This code path created a copy of the inner `ThunkRepr` of a nested thunk, and moved that copy into the `ThunkRepr` of the parent. The effect of this was that the original `ThunkRepr` (unforced!) lived on in the original thunk, without the memoization of the subsequent forcing applying to it. This had the result that Tvix would repeatedly evaluate these thunks without ever memoizing them, if they occured repeatedly as shared inner thunks. Most notably, this would *always* occur when builtins.import was used. What's the solution? ==================== I have completely rewritten `Thunk::force_trampoline_self` to make all flows that can occur in it explicit. I have also removed the outer loop inside of that function, and resorted to more use of trampolining instead. The function is now well-commented and it should be possible to read it from top-to-bottom and get a general sense of what is going on, though the trampolining itself (which is implemented in the VM) needs to be at least partially understood for this. What's the new problem(s)? ========================== One new (known) problem is that we have to construct `Error` instances in all error types here, but we do not have spans available in some thunk-related situations. Due to b/238 we cannot ask the VM for an arbitrary span from the callsite leading to the force. This means that there are now code paths where, under certain conditions, causing an evaluation error during thunk forcing will panic. To fix this we will need to investigate and fix b/238, and/or add a span tracking mechanism to thunks themselves. What other impacts does this have? ================================== With this commit, eval of nixpkgs mostly succeeds (things like stdenv evaluate to the same hashes for us and C++ Nix, meaning we now construct identical derivations without eval breaking). Due to this we progress much further into nixpkgs, which lets us uncover more additional bugs. For example, after this commit we can quickly see that cl/7949 introduces some kind of behavioural issue and should not be merged as-is (this was not apparent before). Additionally, tvix-eval is now seemingly very fast. When doing performance analysis of a nixpkgs eval, we now mostly see the code path for shelling out to C++ Nix to add things to the store in there. We still need those code paths, so we can not (yet) do a performance analysis beyond that. Change-Id: I738525bad8bc5ede5d8c737f023b14b8f4160612 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8012 Tested-by: BuildkiteCI Reviewed-by: flokli --- tvix/eval/src/builtins/impure.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'tvix/eval/src/builtins/impure.rs') diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 3eaebf101e3d..afc85d4c1b40 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/tvix/eval/src/builtins/impure.rs @@ -3,7 +3,6 @@ use smol_str::SmolStr; use std::{ env, - rc::Rc, time::{SystemTime, UNIX_EPOCH}, }; @@ -69,7 +68,7 @@ pub fn impure_builtins() -> Vec<(&'static str, Value)> { result.push(( "storeDir", - Value::Thunk(Thunk::new_suspended_native(Rc::new( + Value::Thunk(Thunk::new_suspended_native(Box::new( |vm: &mut VM| match vm.io().store_dir() { None => Ok(Value::Null), Some(dir) => Ok(Value::String(dir.into())), -- cgit 1.4.1