about summary refs log tree commit diff
path: root/tvix/eval/src/vm (follow)
AgeCommit message (Collapse)AuthorFilesLines
2023-12-12 r/7178 fix(tvix/eval): fix branching on catchable defaults (b/343)Adam Joseph1-0/+7
This commit adds Opcode::OpJumpIfCatchable, which can be inserted ahead of most VM operations which expect a boolean on the stack, in order to handle catchables in branching position properly. Other than remembering to patch the jump, no other changes should be required. This commit also fixes b/343 by emitting this new opcode when compiling if-then-else. There are probably other places where we need to do the same thing. Change-Id: I48de3010014c0bbeba15d34fc0d4800e0bb5a1ef Reviewed-on: https://cl.tvl.fyi/c/depot/+/10288 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7176 feat(tvix/eval): nonrecursive coerce_to_string()Adam Joseph2-4/+4
After this commit, the only non-builtins uses of generators are: - coerce_to_string() uses generators::request_enter_lambda() - Thunk::force() uses generators::request_enter_lambda() That's it! Once those two are taken care of, GenCo can become an implementation detail of `builtins::BuiltinGen`. No more crazy nonlocal flow control within the interpreter: if you've got a GenCo floating around in your code it's because you're writing a builtin, which isn't part of the core interpreter. The interpreter won't need GenCos to talk to itself anymore. Technically generators::request_path_import() is also used by coerce_to_string(), but that's just because the io_handle happens to be part of the VM. There's no recursion-depth issue there, so the call doesn't need to go through the generator mechanism (request_path_import() doesn't call back to the interpreter!) Change-Id: I83ce5774d49b88fdafdd61160975b4937a435bb0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10256 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7175 feat(tvix/eval): nonrecursive deep_force()Adam Joseph2-10/+10
This commit implements deep_force() nonrecursively, by maintaining an explicit stack rather than using the call stack for recursion. As an added bonus, we don't need to pass around the SharedThunkSet anymore, and can in fact completely eliminate SharedThunkSet. Change-Id: I7c4f59f37834d451a28bf6be317eb0a90eac4ee6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10252 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7172 feat(tvix/eval): drop LightSpan::DelayedAdam Joseph1-1/+1
LightSpan::Delayed was introduced in commit bf286a54bc2ac5eeb78c3d5c5ae66e9af24d74d4 which claimed that "This reduces the eval time for `builtins.length (builtins.attrNames (import <nixpkgs> {}))` by *one third*!" I am unable to reproduce this result. In fact, dropping the LightSpan::Delayed variant of the enum makes eval of the same expression slightly faster! I also tried a large evaluation (pkgsCross...hello) and got similar results: slightly faster, slightly less memory. See git footers. I suspect that there was some unrelated horrific inefficiency that has since been fixed. The avoided computation in `get_span()` is nothing more than a binary search! If this were in fact a major performance issue we could simply precompute the mapping from CodeIdx to Span when the Chunk becomes immutable (i.e. at the end of the compilation process, when compiler backtracking is no longer a concern). Since a Span is just 64 bits this is not a space issue, and since binary search is much simpler than compiling Nix expressions it isn't a performance issue either. Technically there is no longer any reason to have LightSpan since it is now a single-variant enum. However there is no rush to remove it, since Rust will optimize its representation into the same thing you'd get if you replaced LightSpan by Span. Prev-Benchmark: {"nixpkgs-attrnames":{"kbytes":"233824","system":"0.32","user":"2.02"}} This-Benchmark: {"nixpkgs-attrnames":{"kbytes":"230192","system":"0.29","user":"2.00"}} Prev-Benchmark: {"pkgsCross.aarch64-multiplatform.hello.outPath":{"kbytes":"458936","system":"0.73","user":"5.36"}} This-Benchmark: {"pkgsCross.aarch64-multiplatform.hello.outPath":{"kbytes":"451808","system":"0.53","user":"5.10"}} Change-Id: Ib9e04806850aa1fc4e66e2a042703986440a7b4e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10254 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-12-12 r/7169 fix(tvix/eval): preserve catchables in nix_cmp_ordering(), fix b/338Adam Joseph1-1/+4
This commit fixes b/338 by properly propagating catchables through comparison operations. Change-Id: I6b0283a40f228ecf9a6398d24c060bdacb1077cf Reviewed-on: https://cl.tvl.fyi/c/depot/+/10221 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7167 feat(tvix/eval): nonrecursive nix_cmp_ordering(), fixes b/339Adam Joseph3-3/+4
This commit rewrites Value::nix_cmp_ordering() into an equivalent nonrecursive form. Except for calls to Thunk::force(), the new form no longer uses generators, and is async only because of the fact that it calls Thunk::force(). I originally believed that this commit would make evaluation faster. In fact it is slightly slower. I believe this is due to the added vec![] allocation. I am investigating. Prev-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460048","system-seconds":"0.68","user-seconds":"5.73"} This-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460224","system-seconds":"0.67","user-seconds":"5.84"} Change-Id: Ic627bc220d9c5aa3c5e68b9b8bf199837cd55af5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10212 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7166 fix(tvix/eval): never use partial_cmp() (partial fix b/338)Adam Joseph1-4/+4
This is part of a fix for b/338. We should never use PartialOrd::partial_cmp(). All Nix types except floats are obviously totally-ordered. In addition, it turns out that because Nix treats division by zero rather than producing a NaN, and because it does not support "negative zero", even floats are in fact totally ordered in Nix. Therefore, every call to PartialOrd::partial_cmp() in tvix is an error. We have to *implement* this function, but we should never call it on built-in types. Moreover, nix_cmp_ordering() currently returns an Option<Ordering>. I'm not sure what was going on there, since it's impossible for it to return None. This commit fixes it to return simply Ordering rather than Option<Ordering>. Change-Id: If5c084164cf19cfb38c5a15554c0422faa5f895d Reviewed-on: https://cl.tvl.fyi/c/depot/+/10218 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-12 r/7164 feat(tvix/eval): nonrecursive nix_eq()Adam Joseph2-8/+8
This commit rewrites Value::nix_eq() into an equivalent. Except for calls to Thunk::force(), the new form no longer uses generators, and is async only because of the fact that it calls Thunk::force(). I believed that the nonrecursive form would be faster. It is, in fact, slightly slower. I believe this is due to the vec![] allocation; I am investigating. Prev-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"459068","system-seconds":"0.71","user-seconds":"5.39"} This-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460048","system-seconds":"0.68","user-seconds":"5.73"} Change-Id: I10f4868891e4b7475df13f0cbc41ec78dd985dd8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10118 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-12-06 r/7120 feat(tvix/eval): rewrite Thunk::force() in nonrecursive formAdam Joseph1-1/+1
This commit rewrites Thunk::force() so that it is not (directly) self-recursive. It maintains a Vec of all the previously-encountered thunks which point to the one it is currently forcing, rather than recursively calling itself. Benefits: - Short term: This commit saves the cost of a round-trip through the generator machinery for the generators::request_force() which is removed by this commit. - Medium term: Once a similar transformation has been applied to nix_cmp(), nix_add(), nix_eq(), and coerce_to_string(), those four functions, along with Thunk::force(), will make non-tail calls only to each other. They can then be merged into a single tail-recursive function which does not use the generator machinery at all: enum Task { Cmp, Add, Eq, CoerceToString, Force}; fn Value::walk(task:Task, v1:Value, v2:Value) { // ... - Long term: The long-term goal here is to use generators **only for builtins** and [Marionette]-style remote control of the VM. In other words: use `async` for things that actually involve concurrency. Calls from the VM to builtins can then be blocking calls, because even cppnix will overflow the stack if you make a MAX_STACK_DEPTH-deep recursive call which passes through a builtin at every stack frame (e.g. `{ func = builtins.sort (a: b: ... func ...) ...}`). This way the inner "tight loop" of the interpreter doesn't pay the costs of `async` and generators. These costs manifest in terms of: performance, complex nonlocal control flow, and language impediments (async Rust is a restricted subset of real Rust, and is missing things like traits). [Marionette]: https://firefox-source-docs.mozilla.org/testing/marionette/Intro.html Change-Id: I6179b8abb2ea0492180fcb347f37595a14665777 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10039 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-11-05 r/6955 chore(tvix): fix trivial clippy lintsVincent Ambo2-2/+2
Relates to b/321. Change-Id: I37284f89b186e469eb432e2bbedb37aa125a6ad4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9961 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su>
2023-09-24 r/6650 fix(tvix/eval): fix b/281 by adding Value::CatchableAdam Joseph2-82/+38
This commit makes catchable errors a variant of Value. The main downside of this approach is that we lose the ability to use Rust's `?` syntax for propagating catchable errors. Change-Id: Ibe89438d8a70dcec29e016df692b5bf88a5cad13 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9289 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-09-24 r/6649 refactor(tvix/eval): factor CatchableErrorKind out of ErrorKindAdam Joseph1-2/+5
This commit creates a separate enum for "catchable" errors (the kind that `builtins.tryEval` can detect). Change-Id: Ie81d1112526d852255d9842f67045f88eab192af Reviewed-on: https://cl.tvl.fyi/c/depot/+/9287 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-09-22 r/6624 docs(tvix/eval): fix some broken docstr referencesFlorian Klink1-2/+2
There's some more left, but they've been renamed/refactored out of sight. Change-Id: I41579dedc74342b4c5f8cb39d2995b5b0c90b0f4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9372 Tested-by: BuildkiteCI Reviewed-by: Connor Brewster <cbrewster@hey.com> Autosubmit: flokli <flokli@flokli.de>
2023-08-20 r/6508 refactor(tvix/eval/vm): don't put HashMap in a BoxFlorian Klink1-1/+1
HashMap already is on the heap. Change-Id: I53763e17469359e85862f297b5c2e7c0d8c3a980 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9104 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-07-22 r/6439 feat(tvix/store/proto): use Bytes instead of Vec<u8>Florian Klink1-2/+2
Makes use of https://github.com/tokio-rs/prost/pull/341, which makes our bytes field cheaper to clone. It's a bit annoying to configure due to https://github.com/hyperium/tonic/issues/908, but the workaround does get the job done. Change-Id: I25714600b041bb5432d3adf5859b151e72b12778 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8975 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: flokli <flokli@flokli.de>
2023-07-21 r/6436 refactor(tvix/store): use bytes for node names and symlink targetsFlorian Klink1-3/+2
Some paths might use names that are not valid UTF-8. We should be able to represent them. We don't actually need to touch the PathInfo structures, as they need to represent StorePaths, which come with their own harder restrictions, which can't encode non-UTF8 data. While this doesn't change any of the wire format of the gRPC messages, it does however change the interface of tvix_eval::EvalIO - its read_dir() method does now return a list of Vec<u8>, rather than SmolStr. Maybe this should be OsString instead? Change-Id: I821016d9a58ec441ee081b0b9f01c9240723af0b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8974 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-06-30 r/6372 chore(tvix/eval/vm): drop unused importFlorian Klink1-1/+1
Change-Id: Ia04778391c198fde21da217bf697aa40157898b0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8846 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2023-06-21 r/6341 fix(tvix/eval): use realpaths for import cachesterni2-3/+39
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 <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-06-20 r/6336 fix(tvix/eval): only finalise formal arguments if defaultingsterni1-13/+17
When dealing with a formal argument in a function argument pattern that has a default expression, there are two different things that can happen at runtime: Either we select its value from the passed attribute successfully or we need to use the default expression. Both of these may be thunks and both of these may need finalisers. However, in the former case this is taken care of elsewhere, the value will always be finalised already if necessary. In the latter case we may need to finalise the thunk resulting from the default expression. However, the thunk corresponding to the expression may never end up in the local's stack slot. Since finalisation goes by stack slot (and not constants), we need to prevent a case where we don't fall back to the default expression, but finalise anyways. Previously, we worked around this by making `OpFinalise` ignore non-thunks. Since finalisation of already evaluated thunks still crashed, the faulty compilation of function pattern arguments could still cause a crash. As a new approach, we reinstate the old behavior of `OpFinalise` to crash whenever encountering something that is either not a thunk or doesn't need finalisation. This can also help catching (similar) miscompilations in the future. To then prevent the crash, we need to track whether we have fallen back or not at runtime. This is done using an additional phantom on the stack that holds a new `FinaliseRequest` value. When it comes to finalisation we check this value and conditionally execute `OpFinalise` based on its value. Resolves b/261 and b/265 (partially). Change-Id: Ic04fb80ec671a2ba11fa645090769c335fb7f58b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8705 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org>
2023-06-11 r/6266 fix(tvix/eval): emit only warnings on shadowed outputsLinus Heckemann1-1/+1
Unfortunately, nixpkgs has at least one case[1] where the out environment variable is shadowed -- though it doesn't cause a problem, since it's shadowed with the correct value, odd as this may be! [1]: https://github.com/NixOS/nixpkgs/blob/c7c298471676ac1c7789ab3c424fbcebecaa6791/pkgs/development/python-modules/pybind11/default.nix#L19 Change-Id: Ibf6790d2484dc9cce8e424feeb5886664d498dc3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8696 Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-06-07 r/6243 fix(tvix/eval): type check function argument with set patternsterni1-0/+13
C++ Nix forces and typechecks the passed argument even if it is not necessary in order to compute the return value of the function. I discovered this when I thought our formals miscompilation might be that we are too strict, but doesn't look like it in this case. Change-Id: Ifb3c92592293052c489d1e3ae8c7c54e4b6b4dc6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8701 Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su>
2023-05-22 r/6173 refactor(tvix/eval): use &Path instead of PathBufFlorian Klink1-3/+3
This allows getting rid of some clones in eval/src/vm/generators.rs. Change-Id: I330390307d3bcfeef19c98954c753ee55b1ccee3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8604 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-05-22 r/6171 refactor(tvix/eval/io): use io::Error instead of tvix_eval errorsFlorian Klink1-6/+29
We didn't return anything useful other than ErrorKind::IO anyways. We can use io::ErrorKind::Unsupported for DummyIO. Fixes b/271. Change-Id: Icb231e9b38168e8b6fa473bfa405d160357b317f Reviewed-on: https://cl.tvl.fyi/c/depot/+/8602 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-05-14 r/6140 feat(tvix/eval/io): allow &mut self in EvalIOFlorian Klink1-1/+1
It's okay if these calls mutate some internal state inside an implementation. Change-Id: I12bb11bde0310778c3da1275696bf7de058863a3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8571 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-03-27 r/6045 refactor(tvix/eval): retain call frames when entering callsVincent Ambo1-6/+13
This grows the frame stack as the call stack grows, which yields *much* better user-facing error messages. I haven't measured the performance impact this has yet, for now I'm still just trying to add more information to errors and then cut down again where necessary. Change-Id: I89f058ef31979edacf4667775d460b60704ce4d7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8334 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su>
2023-03-22 r/6037 feat(tvix/eval): add Evaluation::strict to toggle top-level deepseqVincent Ambo1-3/+6
This makes it possible for callers to control whether they can receive partially evaluated values from an evaluation or not. We're actually flipping the default behaviour to non-strict top-level evaluation, which means that callers have to set `strict = true` on the Evaluation to get the previous behaviour. Change-Id: Ic048e9ba09c88866d4c3177d5fa07db11c4eb20e Reviewed-on: https://cl.tvl.fyi/c/depot/+/8325 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2023-03-17 r/6025 feat(tvix/eval): track span of first force in a thunk blackholeVincent Ambo2-5/+16
This is step 1 towards being able to use all 4 spans that we know when dealing with infinite recursion. It tracks the span at which the force of a thunk was first requested when constructing a blackhole, so that we can highlight the spans of the first and second forces. These are actually the least relevant spans, but the easiest to put in place, more coming soon. Change-Id: I4c7e82f6211b98756439d4148a4191457cc46807 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8269 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-03-17 r/6024 feat(tvix/eval): add generator "name" to NativeError kindVincent Ambo1-3/+8
This produces traces in which we can see what kind of native code was run. Note that these "names" are named after the generator message, so these aren't *really* intended for end-user consumption, but we can give them saner names later. Example: https://gist.github.com/tazjin/82b24e92ace8e821008954867ee05057 This already makes the traces a little easier to parse. Change-Id: Idcd601baf84f492211b732ea0f04b377112e10d0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8268 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su>
2023-03-17 r/6023 feat(tvix/eval): enrich errors with VM's frame stack informationVincent Ambo2-80/+159
When emitting an error at runtime, the VM will now use the new `NativeError` and `BytecodeError` error kinds (which just wrap inner errors) to create a set of diagnostics to emit. The primary diagnostic is emitted last, with `error` type (so it will be coloured red in terminals), the other ones will be emitted with `note` type, highlighting the causal chain. Example: https://gist.github.com/tazjin/25feba7d211702453c9ebd5f8fd378e4 This is currently quite verbose, and we can cut down on this further, but the purpose of this commit is to surface more information first of all before worrying about the exact display. Change-Id: I058104a178c37031c0db6b4b3e4f4170cf76087d Reviewed-on: https://cl.tvl.fyi/c/depot/+/8266 Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2023-03-14 r/5992 chore(tvix): Generator{Request|Response} -> VM{Request|Response}Vincent Ambo2-127/+118
We settled on this being the most reasonable name for this construct. Change-Id: Ic31c45461a842f22aa05f4446123fe3a61dfdbc0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8291 Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5991 chore(tvix/eval): mark async functions which are called by the VMAdam Joseph1-0/+3
Given Rust's current lack of support for tail calls, we cannot avoid using `async` for builtins. This is the only way to avoid overflowing the cpu stack when we have arbitrarily deep builtin/interpreted/builtin/interpreted/... "sandwiches" There are only five `async fn` functions which are not builtins (some come in multiple "flavors"): - add_values - resolve_with - force, final_deep_force - nix_eq, nix_cmp_eq - coerce_to_string These can be written iteratively rather than recursively (and in fact nix_eq used to be written that way!). I volunteer to rewrite them. If written iteratively they would no longer need to be `async`. There are two motivations for limiting our reliance on `async` to only the situation (builtins) where we have no other choice: 1. Performance. We don't really have any good measurement of the performance hit that the Box<dyn Future>s impose on us. Right now all of our large (nixpkgs-eval) tests are swamped by the cost of other things (e.g. fork()ing `nix-store`) so we can't really measure it. Builtins tend to be expensive operations anyways (regexp-matching, sorting, etc) that are likely to already cost more than the `async` overhead. 2. Preserving the ability to switch to `musttail` calls. Clang/LLVM recently got `musttail` (mandatory-elimination tail calls). Rust has refused to add this mainly because WASM doesn't support, but WASM `tail_call` has been implemented and was recently moved to phase 4 (standardization). It is very likely that Rust will get tail calls sometime in the next year; if it does, we won't need async anymore. In the meantime, I'd like to avoid adding any further reliance on `async` in places where it wouldn't be straightforward to replace it with a tail call. https://reviews.llvm.org/D99517 https://github.com/WebAssembly/proposals/pull/157 https: //github.com/rust-lang/rfcs/issues/2691#issuecomment-1462152908 Change-Id: Id15945d5a92bf52c16d93456e3437f91d93bdc57 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8290 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-03-13 r/5990 refactor(tvix/eval): reduce fetch{forced|captured}_with visibilityAdam Joseph1-22/+22
This commit moves fetch_forced_with and fetch_captured_with into the scope of their only caller (resolve_with). Change-Id: I9a8bc27228888729d591e8cb021c431b2b6468f5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8289 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-03-13 r/5988 chore(tvix/eval): prune some dependencies & featuresVincent Ambo1-9/+0
* We no longer need backtrace-on-stack-overflow, as we no longer overflow the stack with the recent eval refactorings. This was weird voodoo anyways, introduced earlier to debug some cases where stack overflows occured. * default features of genawaiter crate are not needed, as we don't use their proc macros Change-Id: I346fc5a18d7f117ee805909a8be8f535b96be76c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8263 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5987 refactor(tvix/eval): reorder bytecode operations match by frequencyVincent Ambo1-265/+264
This reorders the operations in the VM's main `match` statement while evaluating bytecode according to the frequency with which these operations appear in some nixpkgs evaluations. I used raw data that looks like this: https://gist.github.com/tazjin/63d0788a78eb8575b04defaad4ef610d This has a small but noticeable impact on evaluation performance. No operations have changed in any way, this is purely moving code around. Change-Id: Iaa4ef4f0577e98144e8905fec88149c41e8c315c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8262 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2023-03-13 r/5985 docs(tvix/eval): fix reference to `Empty` message in a commentVincent Ambo1-1/+1
Change-Id: I3dc30cca33fbbd8e8686655635ee471f5937d9f8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8257 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5984 refactor(tvix/eval): rename VM::tail_call_value -> VM::call_valueVincent Ambo2-4/+4
The name of this was not accurate anymore after all the recent shuffling, as noted by amjoseph. Conceptual tail calls here only occur for Nix bytecode calling Nix bytecode, but things like a builtin call actually push a new native frame. Change-Id: I1dea8c9663daf86482b8c7b5a23133254b5ca321 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8256 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5983 fix(tvix/eval): emit warnings from builtins.import againVincent Ambo1-7/+28
Wires up generator logic to emit warnings that already have spans attached again. Change-Id: I9f878cec3b9d4f6f7819e7c71bab7ae70bd3f08b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8224 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5982 fix(tvix/eval): more closely line up path resolution with cppnixVincent Ambo1-1/+4
... except now the tests fail, but at least it works Change-Id: I05e86c173f40533ae65548585c1ddaa200ac5235 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8214 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5980 refactor(tvix/eval): VM struct no longer needs to be publicVincent Ambo1-1/+1
Change-Id: I93b485ddd280cc15fcbaecf4aed5fcd22e28a8a8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8212 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5979 fix(tvix/eval): implement cppnix JSON-serialisation semanticsVincent Ambo1-0/+23
This drops the usage of serde::Serialize, as the trait can not be used to implement the correct semantics (function colouring!). Instead, a manual JSON serialisation function is written which correctly handles toString, outPath and other similar weirdnesses. Unexpectedly, the eval-okay-tojson test from the C++ Nix test suite now passes, too. This fixes an issue where serialising data structures containing derivations to JSON would fail. Change-Id: I5c39e3d8356ee93a07eda481410f88610f6dd9f8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8209 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5978 feat(tvix/eval): give generators human-readable namesVincent Ambo3-35/+56
This adds static strings to generator frames that describe the generator in a human-readable fashion, which are then logged in observers. This makes runtime traces very precise, explaining exactly what is being requested from where. Change-Id: I695659a6bd0b7b0bdee75bc8049651f62b150e0c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8206 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5974 refactor(tvix/eval): print only *types* when observing generatorsVincent Ambo1-7/+11
Do not print the entire value (they're likely to be thunks anyways). This is useful because there *can* be cases where something like `nixpkgs` itself is sent through one of these messages, in which case the observer trying to print it will just blow up. Change-Id: I1fa37ea071d75efa0eb3428c6e2fe4351c62be6b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8202 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5969 refactor(tvix/eval): box PathBufVincent Ambo1-2/+2
This shaves another 8 bytes off Value. How did that type get so big?! Change-Id: I65e9b59a1636bd57e3cc4aec5fea16887070b832 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8153 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5964 refactor(tvix/eval): flatten call stack of VM using generatorsVincent Ambo3-51/+1424
Warning: This is probably the biggest refactor in tvix-eval history, so far. This replaces all instances of trampolines and recursion during evaluation of the VM loop with generators. A generator is an asynchronous function that can be suspended to yield a message (in our case, vm::generators::GeneratorRequest) and receive a response (vm::generators::GeneratorResponsee). The `genawaiter` crate provides an interpreter for generators that can drive their execution and lets us move control flow between the VM and suspended generators. To do this, massive changes have occured basically everywhere in the code. On a high-level: 1. The VM is now organised around a frame stack. A frame is either a call frame (execution of Tvix bytecode) or a generator frame (a running or suspended generator). The VM has an outer loop that pops a frame off the frame stack, and then enters an inner loop either driving the execution of the bytecode or the execution of a generator. Both types of frames have several branches that can result in the frame re-enqueuing itself, and enqueuing some other work (in the form of a different frame) on top of itself. The VM will eventually resume the frame when everything "above" it has been suspended. In this way, the VM's new frame stack takes over much of the work that was previously achieved by recursion. 2. All methods previously taking a VM have been refactored into async functions that instead emit/receive generator messages for communication with the VM. Notably, this includes *all* builtins. This has had some other effects: - Some test have been removed or commented out, either because they tested code that was mostly already dead (nix_eq) or because they now require generator scaffolding which we do not have in place for tests (yet). - Because generator functions are technically async (though no async IO is involved), we lose the ability to use much of the Rust standard library e.g. in builtins. This has led to many algorithms being unrolled into iterative versions instead of iterator combinations, and things like sorting had to be implemented from scratch. - Many call sites that previously saw a `Result<..., ErrorKind>` bubble up now only see the result value, as the error handling is encapsulated within the generator loop. This reduces number of places inside of builtin implementations where error context can be attached to calls that can fail. Currently what we gain in this tradeoff is significantly more detailed span information (which we still need to bubble up, this commit does not change the error display). We'll need to do some analysis later of how useful the errors turn out to be and potentially introduce some methods for attaching context to a generator frame again. This change is very difficult to do in stages, as it is very much an "all or nothing" change that affects huge parts of the codebase. I've tried to isolate changes that can be isolated into the parent CLs of this one, but this change is still quite difficult to wrap one's mind and I'm available to discuss it and explain things to any reviewer. Fixes: b/238, b/237, b/251 and potentially others. Change-Id: I39244163ff5bbecd169fe7b274df19262b515699 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8104 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-03-13 r/5963 feat(tvix/eval): implement generator-based Nix equality logicVincent Ambo1-5/+1
Adds a `Value::neo_nix_eq` method (the `neo_` prefix will be dropped when we flip over to the generator implementation of the VM) which implements Nix equality semantics using async, generator-based comparisons. Instead of tracking the "kind" of equality that is being compared (see the pointer-equality doc) through a pair of booleans, I've introduced an enum that explicitly lists the possible comparisons. Change-Id: I3354cc1470eeccb3000a5ae24f2418db1a7a2edc Reviewed-on: https://cl.tvl.fyi/c/depot/+/8241 Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2023-03-07 r/5899 feat(tvix/eval): introduce generators moduleVincent Ambo1-0/+538
This module contains the request/response types for generators requesting actions from the VM. For most of these, an async helper function is added that will be used inside of generator functions to make use of these requests/responses instead of constructing them directly. Change-Id: I1e085f88adaf784a34867957a0e82532d3a83d7c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8148 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
generated by cgit-pink 1.4.1 (git 2.44.2) at 2024-07-07T15·50+0000