about summary refs log tree commit diff
path: root/tvix/eval/src/vm
AgeCommit message (Collapse)AuthorFilesLines
2024-02-21 r/7591 feat(tvix/eval): Store string context alongside dataAspen Smith2-7/+5
Previously, Nix strings were represented as a Box (within Value) pointing to a tuple of an optional context, and another Box pointing to the actual string allocation itself. This is pretty inefficient, both in terms of memory usage (we use 48 whole bytes for a None context!) and in terms of the extra indirection required to get at the actual data. It was necessary, however, because with native Rust DSTs if we had something like `struct NixString(Option<NixContext>, BStr)` we could only pass around *fat* pointers to that value (with the length in the pointer) and that'd make Value need to be bigger (which is a waste of both memory and cache space, since that memory would be unused for all other Values). Instead, this commit implements *manual* allocation of a packed string representation, with the length *in the allocation* as a field past the context. This requires a big old pile of unsafe Rust, but the payoff is clear: hello outpath time: [882.18 ms 897.16 ms 911.23 ms] change: [-15.143% -13.819% -12.500%] (p = 0.00 < 0.05) Performance has improved. Fortunately this change can be localized entirely within value/string.rs, since we were abstracting things out nicely. Change-Id: Ibf56dd16c9c503884f64facbb7f0ac596463efb6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10852 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz> Autosubmit: aspen <root@gws.fyi>
2024-02-21 r/7587 fix(tvix/eval): allow reading non-UTF8 filesFlorian Klink1-1/+1
With our values using bstr now, we're not restricted to only reading files that contain valid UTF-8. Update our `read_to_string` function to `read_to_end` (named like `std::io::Read::read_to_end`), and have it return a Vec<u8>. Change-Id: I87f0291dc855a132689576559c891d66c30ddf2b Reviewed-on: https://cl.tvl.fyi/c/depot/+/11003 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: Pádraic Ó Mhuiris <patrick.morris.310@gmail.com> Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7571 refactor(tvix/eval): add SourceCode directly into error typesVincent Ambo1-4/+16
With this change it's no longer necessary to track the SourceCode struct separately from the evaluation for error reporting: It's just stored directly in the errors. This also ends up resolving an issue in compiler::bindings, where we cloned the Arc containing file references way too often. In fact those clones probably compensate for all additional SourceCode clones during error construction now. Change-Id: Ice93bf161e61f8ea3d48103435e20c53e6aa8c3a Reviewed-on: https://cl.tvl.fyi/c/depot/+/10986 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-02-13 r/7508 feat(tvix/eval): Box Value::CatchableAspen Smith3-8/+8
This is now the only enum variant for Value that is larger than 8 bytes (it's 16 bytes), so boxing it (especially since it's not perf-critical) allows us to get the Value size down to only 16 bytes! Change-Id: I98598e2b762944448bef982e8ff7da6d6683c4aa Reviewed-on: https://cl.tvl.fyi/c/depot/+/10798 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz> Autosubmit: aspen <root@gws.fyi>
2024-02-13 r/7507 revert(tvix/eval): Don't double-box Path valuesAspen Smith1-3/+3
This reverts commit d3d41552cf1f6485f8ebc597a2128a0d15b030a5. This was well-intentioned, but now the boxed Path values are actually the *largest* Value enum variants, at 16 bytes (because they're fat-pointers, with a len) instead of 8 bytes like all the other values. Having the double reference is a reasonable price to pay (it seems; more benchmarks may end up disagreeing) for a smaller Value repr. Change-Id: I0d3e84f646c8f5ffd0b7259c4e456637eea360f7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10797 Tested-by: BuildkiteCI Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-13 r/7506 fix(tvix/eval): Replace inner NixString repr with Box<Bstr>Aspen Smith1-1/+1
Storing a full BString here incurs the extra overhead of the capacity for the inner byte-vector, which we basically never use as Nix strings are immutable (and we don't do any mutation / sharing analysis). Switching to a Box<BStr> cuts us from 72 bytes to 64 bytes per string (and there are a lot of strings!) Change-Id: I11f34c14a08fa02759f260b1c78b2a2b981714e4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10794 Autosubmit: aspen <root@gws.fyi> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-02-10 r/7496 refactor(tvix/eval): Box the inside of Value::JsonAspen Smith1-1/+1
serde_json::Value is pretty large, and is contributing (albeit not exclusively) to the large size of the Value repr. Putting it in a box is *especially* cheap (since it's rarely used) and allows us to (eventually) cut down on the size of Value. Change-Id: I005a802d8527b639beb4e938e3320b11ffa1ef23 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10795 Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-02-09 r/7492 fix(tvix/eval): Propagate catchables in NixAttrs::constructAspen Smith1-2/+4
Correctly propagate the case where the *key* of an attrset is a Value::Catchable (eg { "${builtins.throw "c"}" = "b"; }) in `NixAttrs::construct`, by converting the return type to `Result<Result<Self, CatchableErrorKind>, ErrorKind>` (ugh!!) and correctly handling that everywhere (including an `expect` in the Deserialize impl for NixAttrs, since afaict this is impossible to hit when deserializing from stuff like JSON). Change-Id: Ic4bc611fbfdab27c0bd8a40759689a87c4004a17 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10786 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-02-02 r/7467 refactor(tvix/eval): Box Value::StringAspen Smith2-7/+8
NixString is *quite* large - like 80 bytes - because of the extra capacity value for BString and because of the context. We want to keep Value small since we're passing it around a lot, so let's box the NixString inside Value::String to save on some memory, and make cloning ostensibly a little cheaper Change-Id: I343c8b4e7f61dc3dcbbaba4382efb3b3e5bbabb2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10729 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-01 r/7461 refactor(tvix/eval): Don't double-box Path valuesAspen Smith1-3/+3
PathBuf internally contains a heap pointer (an OsString), so we were in effect double-boxing here. Removing the extra layer by making Tvix::Value represented by a Box<Path> rather than a Box<PathBuf> saves us an indirection, while still avoiding the extra memory overhead of the capacity which was the reason we were boxing PathBuf in the first place. Change-Id: I8c185b9d4646161d1921917f83e87421496a3e24 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10725 Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-01-31 r/7460 fix(tvix): Represent strings as byte arraysAspen Smith1-12/+13
C++ nix uses C-style zero-terminated char pointers to represent strings internally - however, up to this point, tvix has used Rust `String` and `str` for string values. Since those are required to be valid utf-8, we haven't been able to properly represent all the string values that Nix supports. To fix that, this change converts the internal representation of the NixString struct from `Box<str>` to `BString`, from the `bstr` crate - this is a wrapper around a `Vec<u8>` with extra functions for treating that byte vector as a "morally string-like" value, which is basically exactly what we need. Since this changes a pretty fundamental assumption about a pretty core type, there are a *lot* of changes in a lot of places to make this work, but I've tried to keep the general philosophy and intent of most of the code in most places intact. Most notably, there's nothing that's been done to make the derivation stuff in //tvix/glue work with non-utf8 strings everywhere, instead opting to just convert to String/str when passing things into that - there *might* be something to be done there, but I don't know what the rules should be and I don't want to figure them out in this change. To deal with OS-native paths in a way that also works in WASM for tvixbolt, this also adds a dependency on the "os_str_bytes" crate. Fixes: b/189 Fixes: b/337 Change-Id: I5e6eb29c62f47dd91af954f5e12bfc3d186f5526 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10200 Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de> Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-01-18 r/7407 refactor(tvix/eval): generalize EvalIO containerFlorian Klink2-16/+32
Don't restrict to a Box<dyn EvalIO>. There's still one or two places where we do restrict, this will be solved by b/262. Change-Id: Ic8d927d6ea81fa12d90b1e4352f35ffaafbd1adf Reviewed-on: https://cl.tvl.fyi/c/depot/+/10639 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-01-16 r/7395 fix(tvix/eval): lift VM ops over Catchableedef2-115/+102
We want to handle bottoms in a consistent fashion. Previously this was handled by repetitive is_catchable checks, which were not consistently present. Change-Id: I9614c479cc6297d1f64efba22b620a26e2a96802 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10485 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-14 r/7379 fix(tvix/eval): catchable-aware builtinsRyan Lahfa2-25/+49
A bunch of operations in Tvix are not aware of catchable values and does not propagate them. In the meantime, as we wait for a better solution, we just offer this commit for moving the needle. Change-Id: Ic3f0e1550126b0847b597dfc1402c35e0eeef469 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10473 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-03 r/7334 feat(tvix/eval): contextful string coercionRyan Lahfa1-1/+1
String with contexts are always coerced to a string with the same context. Change-Id: I224814febd9cad196bb28876793e76bed564dc72 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10440 Tested-by: BuildkiteCI Autosubmit: raitobezarius <tvl@lahfa.xyz> Reviewed-by: sterni <sternenseemann@systemli.org>
2024-01-03 r/7328 feat(tvix/eval): `${}` propagates contextsRyan Lahfa1-4/+13
We just perform union of contexts of every pieces. Change-Id: Ief925c1818cd8bbec0503e9c625b0630feebfdda Reviewed-on: https://cl.tvl.fyi/c/depot/+/10432 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: raitobezarius <tvl@lahfa.xyz>
2024-01-03 r/7318 chore(tvix/eval): notes on coercion and contextsRyan Lahfa1-0/+3
We make a case for adding a `reject_context` `CoercionKind` here. It does happen during concatenation actually for path concats. Change-Id: I0c196aad917550b9bcd0896cd2127a94f8181ffb Reviewed-on: https://cl.tvl.fyi/c/depot/+/10444 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: raitobezarius <tvl@lahfa.xyz>
2024-01-03 r/7310 feat(tvix/eval): emit warnings with kinds and spansRyan Lahfa1-7/+5
In the past, we had `emit_warning` be no-op and we used `push_warnings` exclusively but as we have consumers of this function, we need it to work somewhat. Change-Id: I78a5ece199a473dec9ef5ea1fae60b36e35137b8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10477 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-12-29 r/7275 fix(tvix/eval): propagate catchables in string interpolationsAdam Joseph1-2/+10
Change-Id: I13d7ce0c7328a8e6fbc6d2c4ff5c4fe6095b96ea Reviewed-on: https://cl.tvl.fyi/c/depot/+/10357 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2023-12-29 r/7274 fix(tvix/eval): catchable in type field of nix_eq()Adam Joseph1-2/+3
Change-Id: I165ff77764e272cc94d18cb03ad6cbc9a8ebefde Reviewed-on: https://cl.tvl.fyi/c/depot/+/10348 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2023-12-29 r/7272 refactor(tvix/eval): let OpCoerceToString select the CoercionKindAdam Joseph1-9/+2
Change-Id: I92d58ef216d7e0766af70f019b3dcd445284a95d Reviewed-on: https://cl.tvl.fyi/c/depot/+/10344 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-29 r/7271 fix(tvix/eval): add stack depth assertion to OpReturnAdam Joseph1-0/+7
I'm still trying to work out the exact stack invariants for tvix. We really should add assertions for them; getting the stack messed up is no fun. This commit adds one simple assertion. It also adds a missing stack-push (my mistake) in one place, which was uncovered by the assertion. Change-Id: I9d8b4bd1702d954e325832c5935b0d7e3eb68422 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10369 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-14 r/7218 fix(tvix/eval): remove incorrect imports when coercingsterni2-12/+68
The default behavior of string coercion in C++ Nix is to weakly coerce and import to store if necessary. There is a flag to make it strongly coerce (coerceMore) and a flag that controls whether path values have the corresponding file/directory imported into the store before returning the (store) path as a string (copyToStore). We need to implement our equivalent to the copyToStore (import_paths) flag for the benefit of weak coercions that don't import into the store (dirOf, baseNameOf, readFile, ...) and strong coercions that don't import into the store (toString). This makes coerce_to_string as well as CoercionKind weirder and more versatile, but prevents us from reimplementing parts of the coercion logic constantly as can be seen in the case of baseNameOf. Note that it is not possible to test this properly in //tvix/eval tests due to the lack of an appropriate EvalIO implementation being available. Tests should be added to //tvix/glue down the line. Change-Id: I8fb8ab99c7fe08e311d2ba1c36960746bf22f566 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10361 Autosubmit: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2023-12-14 r/7217 fix(tvix/eval): determine meaning of `+` exprs based on first typesterni1-5/+11
This matches the behavior of C++ Nix more closely where the decision is made based on the first type based to ExprConcatStrings: https://github.com/NixOS/nix/blob/1f93fa2ed29ff0ba92bfa58995232668187fb0d0/src/libexpr/eval.cc#L1967-L2025 Note that this doesn't make a difference in any successful evaluation (at least to my knowledge), but ensures that our error messages will match C++ Nix more closely, e.g. in the case of `1 + "string"`. Change-Id: I8059930788f9c8d98baf98e3d93d8a060ef961f2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10360 Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7206 refactor(tvix/eval): vm::add_values(): be less verboseAdam Joseph1-12/+8
Change-Id: Icf328649fd70bdf0fc3ba6cd7754ae29735f11f7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10035 Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-12-12 r/7189 fix(tvix/eval): `?`: propagate catchablesAdam Joseph1-8/+16
This commit fixes out `?` operator so it correctly propagates catchables. Change-Id: Iebaa153a8492101ee3ddd29893c98730ff331547 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10317 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-12 r/7188 feat(tvix/eval): OpAttrsSelect should propagate catchablesAdam Joseph1-11/+19
Previously, using a catchable as either argument of OpAttrsSelect would result in an unrecoverable error. This commit matches cppnix behavior by propagating the catchable. Change-Id: I4877f4068ec2b823225f185290693c101d0b9c9e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10303 Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su>
2023-12-12 r/7187 fix(tvix/eval): calling a catchable is catchableAdam Joseph1-0/+6
When attempting to call a Value, if it is a Value::Catchable we must not cause an uncatchable failure. This commit simply reuses the Value::Catchable as the result of attempting to call it. This is safe because nix is designed so that nix code cannot distinguish between different catchable failures -- they all look the same to the interpreted code. This fixes b/351. Change-Id: Ibf763a08753e541843626182ff59fdbf15ea2959 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10300 Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-12-12 r/7184 fix(tvix/eval): fix testing catchables for inequalityAdam Joseph1-2/+7
Fixes b/347. Change-Id: Icad0251884d4d8adcdf8d690b91385bf4896f9c8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10294 Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su>
2023-12-12 r/7182 fix(tvix/eval): handle catchables in attribute set updatesAdam Joseph1-4/+11
Fixes b/346. Change-Id: I277121d2363e605ebe09651ed9440fe1bc126c8c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10292 Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su>
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
generated by cgit-pink 1.4.1 (git 2.44.2) at 2024-11-11T14·31+0000