From 66047063e02be3188a558958de8938c9015c0f89 Mon Sep 17 00:00:00 2001 From: sterni Date: Tue, 20 Jun 2023 16:21:26 +0200 Subject: fix(tvix/eval): use realpaths for import cache I've noticed this behavior when writing the admittedly cursed test case included in this CL. Alternatively we could use some sort of machinery using `builtins.trace`, but I don't think we capture stderr anywhere. I've elected to put this into the eval cache itself while C++ Nix does it in builtins.import already, namely via `realisePath`. We don't have an equivalent for this yet, since we don't support any kind of IfD, but we could revise that later. In any case, it seems good to encapsulate `ImportCache` in this way, as it'll also allow using file hashes as identifiers, for example. C++ Nix also does our equivalent of canon_path in `builtins.import` which we still don't, but I suspect it hardly makes a difference. Change-Id: I05004737ca2458a4c67359d9e7d9a2f2154a0a0f Reviewed-on: https://cl.tvl.fyi/c/depot/+/8839 Autosubmit: sterni Reviewed-by: tazjin Tested-by: BuildkiteCI --- tvix/eval/src/compiler/import.rs | 1 + .../tvix_tests/eval-okay-observable-eval-cache.exp | 1 + .../tvix_tests/eval-okay-observable-eval-cache.nix | 7 ++++ .../tests/tvix_tests/observable-eval-cache1.nix | 1 + .../tests/tvix_tests/observable-eval-cache2.nix | 1 + .../tests/tvix_tests/observable-eval-cache3.nix | 1 + tvix/eval/src/vm/generators.rs | 2 +- tvix/eval/src/vm/mod.rs | 40 ++++++++++++++++++++-- 8 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix create mode 100644 tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix create mode 120000 tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix create mode 100644 tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix (limited to 'tvix/eval') diff --git a/tvix/eval/src/compiler/import.rs b/tvix/eval/src/compiler/import.rs index 7e1aabbf1f8b..59e3d92e8127 100644 --- a/tvix/eval/src/compiler/import.rs +++ b/tvix/eval/src/compiler/import.rs @@ -24,6 +24,7 @@ async fn import_impl( source: SourceCode, mut args: Vec, ) -> Result { + // TODO(sterni): canon_path()? let mut path = coerce_value_to_path(&co, args.pop().unwrap()).await?; if path.is_dir() { diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp new file mode 100644 index 000000000000..aaa53b602586 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp @@ -0,0 +1 @@ +[ true true false false true ] diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix new file mode 100644 index 000000000000..24003d0637ae --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix @@ -0,0 +1,7 @@ +[ + (import ./observable-eval-cache1.nix == import ./observable-eval-cache1.nix) + (import ./observable-eval-cache1.nix == import ./observable-eval-cache2.nix) + (import ./observable-eval-cache1.nix == import ./observable-eval-cache3.nix) + (import ./observable-eval-cache2.nix == import ./observable-eval-cache3.nix) + (import ./observable-eval-cache3.nix == import ./observable-eval-cache3.nix) +] diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix new file mode 100644 index 000000000000..b5f3f59a792d --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix @@ -0,0 +1 @@ +let id = x: x; in { inherit id; } diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix new file mode 120000 index 000000000000..7f69c0eb47eb --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix @@ -0,0 +1 @@ +observable-eval-cache1.nix \ No newline at end of file diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix new file mode 100644 index 000000000000..b5f3f59a792d --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix @@ -0,0 +1 @@ +let id = x: x; in { inherit id; } diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index 138600be194a..fb60a45f20cc 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -393,7 +393,7 @@ impl<'o> VM<'o> { } VMRequest::ImportCacheLookup(path) => { - if let Some(cached) = self.import_cache.get(&path) { + if let Some(cached) = self.import_cache.get(path) { message = VMResponse::Value(cached.clone()); } else { message = VMResponse::Empty; diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index d004af3c6f45..cc66ee4b6f36 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -14,7 +14,7 @@ mod macros; use codemap::Span; use serde_json::json; -use std::{cmp::Ordering, collections::HashMap, ops::DerefMut, path::PathBuf, rc::Rc}; +use std::{cmp::Ordering, collections::HashMap, ops::DerefMut, path::Path, path::PathBuf, rc::Rc}; use crate::{ arithmetic_op, @@ -218,6 +218,42 @@ impl Frame { } } +#[derive(Default)] +struct ImportCache(Box>); + +/// The `ImportCache` holds the `Value` resulting from `import`ing a certain +/// file, so that the same file doesn't need to be re-evaluated multiple times. +/// Currently the real path of the imported file (determined using +/// [`std::fs::canonicalize()`], not to be confused with our +/// [`value::canon_path()`]) is used to identify the file, just like C++ Nix +/// does. +/// +/// Errors while determining the real path are currently just ignored, since we +/// pass around some fake paths like `/__corepkgs__/fetchurl.nix`. +/// +/// In the future, we could use something more sophisticated, like file hashes. +/// However, a consideration is that the eval cache is observable via impurities +/// like pointer equality and `builtins.trace`. +impl ImportCache { + fn get(&self, path: PathBuf) -> Option<&Value> { + let path = match std::fs::canonicalize(path.as_path()).map_err(ErrorKind::from) { + Ok(path) => path, + Err(_) => path, + }; + self.0.get(&path) + } + + fn insert(&mut self, path: PathBuf, value: Value) -> Option { + self.0.insert( + match std::fs::canonicalize(path.as_path()).map_err(ErrorKind::from) { + Ok(path) => path, + Err(_) => path, + }, + value, + ) + } +} + struct VM<'o> { /// VM's frame stack, representing the execution contexts the VM is working /// through. Elements are usually pushed when functions are called, or @@ -241,7 +277,7 @@ struct VM<'o> { /// Import cache, mapping absolute file paths to the value that /// they compile to. Note that this reuses thunks, too! // TODO: should probably be based on a file hash - pub import_cache: Box>, + pub import_cache: ImportCache, /// Parsed Nix search path, which is used to resolve `<...>` /// references. -- cgit 1.4.1