about summary refs log tree commit diff
path: root/tvix/eval
AgeCommit message (Collapse)AuthorFilesLines
2024-05-26 r/8175 fix(tvix/eval/io): OsStringExt and std::fs::File import are impure-onlyFlorian Klink1-2/+4
These are only needed when building with the impure feature enabled. This removes some warnings when building with --no-default-features. Change-Id: I3139d9133d4846aeb1b1b5f3830c0d078d047292 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11720 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-05-25 r/8168 fix(tvix/eval): drop superfluous string context checkedef1-4/+1
cl/11712 simultaneously introduced this check and made it unnecessary, since NixString::context should never return `Some` for empty contexts now. Change-Id: I41a655ff33910e8326cbb7d7526eb91bd19e9585 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11713 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-05-25 r/8167 fix(tvix/eval): disallow empty but allocated string contextsedef1-3/+25
Both `Some(NixContext::new())` and `None` represent empty contexts, but the former trips up `NixString::has_context`, and seems likely to trip up other things. We could hide the difference in the accessors, but we don't really *want* the distinction to exist, since heap-allocating a null value is pretty much always a mistake. Change-Id: Ie84d26fb0d4b59e68354891ba13bde3bae40ab6e Reviewed-on: https://cl.tvl.fyi/c/depot/+/11712 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-05-23 r/8164 feat(tvix/eval): rm NixContext::join, add take_context & IntoIteratorFlorian Klink5-58/+55
In places where we want to extend context with that from another NixString, use take_context() to split it off, then call .extend(), making use of IntoIterator to avoid a bunch of clones. Change-Id: I2460141a3ed776c64c36132b2203b6a1d710b922 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11705 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: edef <edef@edef.eu>
2024-05-23 r/8163 feat(tvix/eval): teach builtins.toXML contextFlorian Klink2-6/+38
XmlEmitter gains a NixContext field, and `write_typed_value` extends it with all context elements present in the passed value. Once all serialization is done, a into_context() function returns the collected context, so we can construct a NixString with context. Tests for this live in tvix-glue, as we use builtins.derivation, which is not present in the tvix-eval crate. Fixes b/398. Change-Id: I85feaaa17b753885f8a017a54e419ec4e602af21 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11704 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de> Autosubmit: flokli <flokli@flokli.de> Reviewed-by: Alyssa Ross <hi@alyssa.is>
2024-05-23 r/8162 feat(tvix/eval): add NixContext::extendFlorian Klink1-1/+9
This is a slightly less annoying version of `join`, which does not consume self. It's more consistent with HashSet::extend(). Change-Id: Ifd0872da36fe8e7b2aa6948674cb8e4023abe9d7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11703 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: edef <edef@edef.eu>
2024-05-23 r/8161 fix(tvix/eval): NixString are bytesFlorian Klink1-1/+1
This is not necessarily valid UTF-8. Change-Id: I72f3157240772eb9c558e5699b4785e44d256fd4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11702 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2024-05-22 r/8160 refactor(tvix/eval): rewrite xml emitter to be simple-stupidProfpatsch7-66/+205
In order to be compatible with the nix XML generator, it’s easier to generate the XML directly, instead of going through a library which we have to bend to do what we need. Removes dependency on `xml-rs`, which came with a full XML parser that we didn’t use. Only takes a tiny bit of code for the XML escaping, somewhat simplified. I add a little escaping value, to make sure we have the same behaviour as nix proper. Interestingly enough, we never need to escape XML attribute names, because the `builtins.toXML` format encodes user-defined values as attribute keys only. So we only escape attribute values. Fixes: https://b.tvl.fyi/issues/399 Change-Id: If4d407d324864b3bb9aa3160e2ec6889f7727127 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11697 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de> Autosubmit: Profpatsch <mail@profpatsch.de>
2024-05-20 r/8156 fix(tvix/eval): use fake values for __curPos, rather than an errorFlorian Klink1-2/+9
Have this return the same values as builtins.unsafeGetAttrsPos, rather than returning a CatchableErrorKind, which crashes the CLI if it bubbles up. The environment we're in doesn't allow emitting a warning, as we don't have `co` in scope, but that's probably OK as a stopgap solution. Alternative to cl/11665. Change-Id: I5b2c2530842547c93b6533ed9601ee9b2923b1bf Reviewed-on: https://cl.tvl.fyi/c/depot/+/11685 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-05-03 r/8072 docs(tvix/eval): add document about how binding construction worksVincent Ambo1-0/+133
Change-Id: I216be1b75eb9f18a58ab2164a77b3c51de8bf784 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11583 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2024-04-30 r/8038 test(tvix): Fix tvix tests on macOSAlice Carroll1-1/+8
Prior to this, some tests would not build or would fail in an obscure way. Change-Id: I68587cc7592492ebfd71ca02fc7ccc9ff7c0196f Reviewed-on: https://cl.tvl.fyi/c/depot/+/11544 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-04-28 r/8025 chore(3p/sources): Bump channels & overlayssterni2-2/+2
- //tvix: address new clippy lints - //users/tazjin: Satisfy gonic module's new need for a playlist folder. - //users/aspen/games: adjust for changed location of df's default init.txt and d_init.txt. Change-Id: I00a2adb506ae866206fb6f88c39c9a6af320380f Reviewed-on: https://cl.tvl.fyi/c/depot/+/11509 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI Reviewed-by: aspen <root@gws.fyi>
2024-04-22 r/7991 fix(tvix/eval): don't impl From<NixString> for StringFlorian Klink1-5/+1
This caught me by accident in an earlier revision of cl/11500 - I had a `NixString`, wanted to return it as a `String`, so I was naively calling `s.into()`. That unfortunately gave me the `Display` implementation of `NixString`, which quotes strings, causing an annoying error further up the stack. NixStrings are bytes, we can keep the impl From<NixString> for BString, but having a `.into()` suddenly do quoting is more than unexpected. Change-Id: I5434ba94bfe6c493d0a57e68225ecc22daa4b948 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11505 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-04-13 r/7909 fix(tvix/eval): fix the `unsafeGetAttrPos` stub return valueRyan Lahfa1-1/+1
It was returning `{ line, col, file }` instead of `{ line, column, file }`. Note this is not a regression in Nix, Nix always had `column` since 2013. Change-Id: I4871a201b32324dff0432526d0449806a535828f Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz> Reviewed-on: https://cl.tvl.fyi/c/depot/+/11409 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-04-13 r/7899 feat(tvix/eval): contextful JSON operationsRyan Lahfa5-26/+63
`toJSON` transform a Nix structure into a JSON string. For each context in that Nix structure, the JSON string must possess it. Thus, it is necessary to take the union of all contexts and attach it to the final structure. Unfortunately, the return type of `into_json` is a serde's JSON object, not a string. Therefore, it is not possible to reuse `NixString` machinery. Context tests are reinforced as Nix does not test those behaviors. Fixes b/393. Change-Id: I5afdbc4e18dd70469192c1aa657d1049ba330149 Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz> Reviewed-on: https://cl.tvl.fyi/c/depot/+/11266 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-04-13 r/7895 docs(tvix/eval): remove link to cargo manifest referenceFlorian Klink1-2/+0
We don't have it in all Cargo.toml files either. Change-Id: Ifcb333a39603a7e402ab0068e0f0f1b5af769e13 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11388 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Reviewed-by: Brian Olsen <me@griff.name> Autosubmit: flokli <flokli@flokli.de>
2024-04-12 r/7885 chore(tvix): bump rstest to 0.19.0Florian Klink1-1/+1
Change-Id: Ib2f5e84fdb8be1210b3507da67d4fe84f061651e Reviewed-on: https://cl.tvl.fyi/c/depot/+/11387 Tested-by: BuildkiteCI Reviewed-by: Brian Olsen <me@griff.name> Autosubmit: flokli <flokli@flokli.de> Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-04-09 r/7882 fix(tvix): Avoid buffering file into memory in builtins.hashFileConnor Brewster6-41/+51
Right now `builtins.hashFile` always reads the entire file into memory before hashing, which is not ideal for large files. This replaces `read_to_string` with `open_file` which allows calculating the hash of the file without buffering it entirely into memory. Other callers can continue to buffer into memory if they choose, but they still use the `open_file` VM request and then call `read_to_string` or `read_to_end` on the `std::io::Reader`. Fixes b/380 Change-Id: Ifa1c8324bcee8f751604b0b449feab875c632fda Reviewed-on: https://cl.tvl.fyi/c/depot/+/11236 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-04-04 r/7851 feat(tvix/eval): implement `appendContext`Ryan Lahfa2-1/+81
`appendContext s ctx` will just append a user-crafted context attrs to `s`. The most important part of this builtin is to perform all the relevant invariant validations to avoid letting the user craft invalid contexts which can never be built, e.g. invalid store paths, inexistent derivations, etc. This version is incomplete and full of TODOs, but passes all the Nix's context strings tests, so we turn them on. Change-Id: I625dc5e7c4f5b784f078b390f04b0ee5a8d65a7c Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz> Reviewed-on: https://cl.tvl.fyi/c/depot/+/11263 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-03-31 r/7806 feat(tvix/eval): implement `unsafeDiscardOutputDependency`Ryan Lahfa3-7/+43
This builtin only transforms any `NixContextElement::Derivation` into the trivial `NixContextElement::Plain`. This is a forgetful functor on derivation-deep context strings. The test coverage of this change is done in cl/11264. Change-Id: Icd00778c97766be6db8a6bdabaa59e9724353ec5 Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz> Reviewed-on: https://cl.tvl.fyi/c/depot/+/11262 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-03-28 r/7801 feat(tvix/eval): non-implement `unsafeDiscardOutputDependency`Ryan Lahfa1-0/+13
Nixpkgs make use of it and this is a 2.3 builtin that was missed. Change-Id: If36d748eed4a5f777344556c0802392c76c8d53c Signed-off-by: Ryan Lahfa <tvl@lahfa.xyz> Reviewed-on: https://cl.tvl.fyi/c/depot/+/11260 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-03-19 r/7736 fix(tvix/eval): propagate errs from key comparison in genericClosuresterni3-7/+17
The accompanying test case shows that we need to bubble up the catchable error from the equality check if one is created. Change-Id: Ic9929a57aa7653c8aa5a72d1711cf3264798c731 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11159 Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su>
2024-03-19 r/7735 fix(tvix/eval): don't force lambda arg in map, mapAttrs & genListsterni7-3/+34
It is pretty pointless to force the function argument if we are going to use a suspended call later since forcing the function may fail in ways that are not covered by Catchables (non-recoverable errors, infinite recursions). From this, it kind of seems as if using #[catch] is never correct and should be replaced by #[lazy]. Also we should probably try to come up with more test cases for stuff where laziness gets us out of the jam as an equivalent to the catchable tests for nonrecoverable errors. Fixes b/386. Change-Id: Ia926df4ac1b440ec430403ab7b40924a0c97221b Reviewed-on: https://cl.tvl.fyi/c/depot/+/11153 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org>
2024-03-16 r/7704 fix(tvix/eval): builtins.unsafeGetAttrPos returns path as a stringsterni1-1/+1
Change-Id: Ia1058a5ea687f413fbb62365da199032bdbd3fd2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11155 Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org>
2024-03-15 r/7700 docs(tvix): fix some docstringsFlorian Klink1-1/+1
Change-Id: Ife599387d0472cd746b992bd6755a2fb6a0e0dc4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11158 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: Connor Brewster <cbrewster@hey.com> Tested-by: BuildkiteCI
2024-03-15 r/7699 fix(depotfmt): only exclude *imported* tvix testsVincent Ambo90-271/+411
When we added the Nix language test suite in cl/6126, we excluded the whole tvix tests folder from Nix formatting. This is unintentional, as we probably want *our* tests to be formatted correctly. Change-Id: I2b66d79e30fae17e75d5a1f8c44e279886091c5f Reviewed-on: https://cl.tvl.fyi/c/depot/+/11154 Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-03-12 r/7687 docs(tvix/eval): C++ Nix now has deduplicated inherit (from)sterni1-0/+3
Change-Id: Icf74c699130c2ab774973a7e98bc44ffba8c5ec2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11139 Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: flokli <flokli@flokli.de>
2024-02-29 r/7625 feat(tvix/eval): implement `builtins.hashFile`Padraic-O-Mhuiris6-25/+42
The primary addition is the `builtin_hashFile` function which attempts value-path coercion and on success reads the file to a nix_string. Similar to the `builtins_hashString` implementation, the hash is then derived using the `hash_nix_string` function in the introduced hash module which has extracted hashing functionality initially specified in cl/11005. Change-Id: I5e3ef0317223af99488ebb79efc2fb49b4cbc1b6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11007 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-02-24 r/7601 chore: update links to tvixboltFlorian Klink1-1/+1
Use bolt.tvix.dev everywhere. Change-Id: Ifebbea60058418c12cde20a2a3879a8a7f5f830c Reviewed-on: https://cl.tvl.fyi/c/depot/+/11022 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-02-23 r/7597 feat(tvix/eval): implement `builtins.hashString`Padraic-O-Mhuiris11-11/+52
Implements md5, sha1, sha256 and sha512 using the related crates from the RustCrypto hashes project (https://github.com/RustCrypto/hashes) Change-Id: I00730dea44ec9ef85309edc27addab0ae88814b8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11005 Tested-by: BuildkiteCI Reviewed-by: aspen <root@gws.fyi>
2024-02-21 r/7591 feat(tvix/eval): Store string context alongside dataAspen Smith7-63/+365
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 Klink2-6/+6
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-21 r/7586 docs(tvix/eval): notes on the implementation of catchable errorssterni1-0/+131
Change-Id: I3b2f039d2c2d40ee5d13d071915ced6f3deb18f2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11000 Autosubmit: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-02-20 r/7580 fix(tvix/eval): Propagate catchables in builtins.groupByAspen Smith3-4/+6
One last place where we needed to wrap the forcing of the element of a list in `try_value!`. This fixes a previously `notyetpassing` test Change-Id: I8827a3e39630e6959013b70bdfa9cbcb93f4e91c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10789 Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2024-02-20 r/7577 fix(tvix/eval): propagate catchable errors at the top of an evalVincent Ambo3-6/+46
(Re-)Adds an error variant that wraps a catchable error kind, which is used for returning the result of an evaluation. Previously this would return the internal catchable value, which would lead to panics if users tried to use these. Somehow this was missed; I think we need error output tests. Change-Id: Id6e24aa2ce4ea4358a29b2e1cf4a6749986baf8c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10991 Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7574 fix(tvix/eval): make path resolution work by default in impure modeVincent Ambo1-0/+6
The previous behaviour (enabling `import`, but not allowing e.g. `<nixpkgs/lib>` to resolve) was very confusing. Now imports from NIX_PATH become enabled by default, unless the user already overrode that behaviour with something else by setting `Evaluation::nix_path` manually. Change-Id: Iad970beb633d9887be4b185b01e6f5858d81bea3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10993 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-02-20 r/7573 fix(tvix/eval): fix accidental recursion in printing errorsVincent Ambo1-1/+1
There's some code path where fancy_format_str()->Error::Display, which recurses forever and overflows the stack. This was introduced in a previous commit today. Change-Id: I87a59492099f6c138c752478901b9aa614bb57cc Reviewed-on: https://cl.tvl.fyi/c/depot/+/10990 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: Peter Kolloch <info@eigenvalue.net>
2024-02-20 r/7572 refactor(tvix/eval): use internal SourceCode field in error printersVincent Ambo1-17/+16
Makes use of the SourceCode field now being stored directly in errors (see parent CL). With this change, the default `Display` implementation can now format errors correctly, and there is no need to keep a `SourceCode` around just for error formatting. Updates dependent crates (CLI, serde, tvixbolt) to use this correctly. Change-Id: Iddc5d7a6b4bab391f30a999e4c68aca34304c059 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10987 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7571 refactor(tvix/eval): add SourceCode directly into error typesVincent Ambo6-40/+67
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-20 r/7569 refactor(tvix/eval): add API for enabling impure evaluation featuresVincent Ambo1-10/+20
This makes some of the work of configuring an arbitrary I/O handler easier. Change-Id: I158db3235fe83df6e709578ed515e0e028c20086 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10959 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-02-13 r/7509 test(tvix/eval): Add test asserting size of ValueAspen Smith1-0/+8
Now that I've done a ton of things to make sure Value is small on the stack (16 bytes, which is a perfectly reasonable size for a programming language Value enum), add a test asserting it stays that way. These size improvements have a measurable impact, too - here's the `hello outpath` benchmark compared between canon (as of r/7495) and this commit: hello outpath time: [990.56 ms 995.83 ms 1.0070 s] change: [-7.1397% -6.1302% -5.1651%] (p = 0.00 < 0.05) Performance has improved. Change-Id: If99a0976eab28eb5e516fcd2f4a0e068145af23e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10799 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-13 r/7508 feat(tvix/eval): Box Value::CatchableAspen Smith10-32/+37
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 Smith6-23/+20
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 Smith3-49/+79
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/7497 refactor(tvix/eval): Box the strings in CatchableErrorKindAspen Smith3-13/+14
These strings are allocated once and never changed, so they don't need the additional overhead of a capacity given by String - instead, we can use Box<str> and save on 16 bytes for each of these, *and* for each Value since this is currently the largest Value variant. Change-Id: I3e5cb070fe6c5bf82114c92d04f6bae775663a7e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10796 Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2024-02-10 r/7496 refactor(tvix/eval): Box the inside of Value::JsonAspen Smith3-3/+3
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-10 r/7495 feat(tvix/eval): strengthen significantly catchable test suiteRyan Lahfa71-0/+203
Adds a bunch (notably certain overlapping) tests for catchable situations. This should cover many scenarios, argument is catchable, element in argument is catchable, function returns catchable in the middle of the processing, etc. Co-authored-by: Aspen Smith <root@gws.fyi> Change-Id: Icd722cf8dbc91a24f45cd540a328711e5826f76c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10621 Reviewed-by: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-02-09 r/7492 fix(tvix/eval): Propagate catchables in NixAttrs::constructAspen Smith5-13/+31
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-08 r/7488 fix(tvix/eval): Inline List.sort_by, and propagate errorsAspen Smith2-53/+45
In order to correctly propagate errors in the comparator passed to builtins.sort, we need to do all the sorting in a context where we can short-circuit return `Value`s (because catchables are Values on the `Ok` side of the Result , not `Err`s). Unfortunately this means we have to *inline* the List `sort_by` implementation into the builtin_sort function - fortunately this is the only place that was called so this is relatively low cost. This does that, and adds the requisite `try_value!` invocation to allow us to propagate comparator errors here. As before, this doesn't include tests, primarily since those are coming in the next commit. Change-Id: I8453c3aa2cd82299eae89828e2a2bb118da4cd48 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10754 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-02-08 r/7487 fix(tvix): Catch errors for generator in some builtinsAspen Smith1-2/+3
Nix doesn't propagate errors for the function argument to some builtins, like genList and map: ❯ nix repl Welcome to Nix version 2.3.17. Type :? for help. nix-repl> (builtins.tryEval (builtins.genList (builtins.throw "a") 10)).success true nix-repl> (builtins.tryEval (builtins.map (builtins.throw "a") [ "" ])).success true Note that this is untested as of this particular commit, only because a big test suite covering all sorts of catchable error propagation issues is coming next Change-Id: I48c8eb390a541204b1a6d438c753fa1ca9b3877e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10753 Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>