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>
This switches out the previous compressed representation (count of
instructions per span) with a representation where the chunk's span
list stores the index of the first operation that belongs to a span,
and finds the right span by using a binary search when looking them
up.
This improves the lookup complexity from O(n) to O(log n).
This improvement was suggested and (mostly) implemented by GPT-4. I
only fixed up some names and updated the logic for deleting
spans (which it only did not do because I didn't tell it about that).
The code was verified by producing a complex error before/after the
change and ensuring that all spans in the error match exactly.
Co-Authored-By: GPT-4
Change-Id: Ibfa12cc6973af1c9b0ae55bb464d1975209771f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8385
Reviewed-by: ezemtsov <eugene.zemtsov@gmail.com>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
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>
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>
Emits the span of the `set` that is being accessed in the `force`
operation of an attribute access.
Looking at traces, it's a lot more useful to get information about
*what* is being forced, as in cases like `foo.bar` it can be
misleading to have an error highlight `bar`, when the error occured
while forcing `foo` to be able to access `bar` in the first place.
Change-Id: Id46ff28f20c67cb4971727ac52cc4811795cea2d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8272
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This reports the span
1. of the code within a thunk,
2. of the place where the thunk was instantiated,
3. of the place where the thunk was first forced,
4. of the place where the thunk was forced again,
when yielding an infinite recursion error, which hopefully makes it
easier to debug them.
The spans are tracked in the ThunkRepr::Blackhole variant when putting
a thunk under evaluation.
Note that we currently have some loss of span precision in the VM loop
when switching between frame types, so spans 3/4 are currently a bit
wonky. Working on it.
Change-Id: Icbd2a9df903d00e8c2545b3fc46dcd2a9e3e3e55
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8270
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
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>
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>
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
This actually uses coercion under the hood in C++ Nix. See the test
for an example.
Change-Id: Id56b364acf269225b6829d0b600e0222f8b3608d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8322
Reviewed-by: andi <andi@notmuch.email>
Tested-by: BuildkiteCI
This was commented out and forgotten during the generator refactor, oh
well.
Change-Id: I474b685159a955a846db462da0dd0067af177b04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8321
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Change-Id: I8224bf039f739c401900b5a2ddc839810c87cf6e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8226
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
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>
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/D99517https://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>
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
This rewrites nix_cmp_ordering as an iterative loop, which
eliminates the extra pinned-boxing helper function.
Change-Id: I33d0ecc913e02affd8fd4c7bc1c9ecfdf4c7deb9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8288
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
* 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>
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
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>
... 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
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
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>
These are serialised as the serialisation of the value of that field.
Change-Id: Ida51708b1f43ce09b0ec835f4e265918aa31dd09
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8205
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
These must be serialised to a JSON string of the *result* of coercing
the function application to a string.
Change-Id: Ib7f49ccd950503ddbdbf99643cd59565e26b50da
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8204
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
It turns out that this is used not just in coerceToString, but also in
toJSON.
Change-Id: I1c324b115a0b8bb6d83446d5bf70453c9b90685e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8203
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
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>
Print only the top 6 values of the stack, not the entire stack.
There's very few operations that deal with more values anyways, so the
rest are not likely to be useful.
This gets us one step closer to tracing VERY large executions without
blowing up.
Change-Id: I97472321b0321b25d534d9f53b3aadfacc2318fa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8201
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This can actually blow up when tracing arbitrary execution, as some of
the data structures just get too large to run through a tabwriter.
Change-Id: I6ec4c30ee48655b8a62954ca219107404fb2c256
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8200
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Formals can be initialised with deferred default values (see the test
cases), in which case they need an extra thunk to have something that
can be finalised appropriately when the setup is done.
Fixes: b/255
Change-Id: I380e3770be68eaa83ace96d450c7cead32dacc9f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8196
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
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
No longer needed, and in some cases caused some extra work.
Change-Id: I64e8e7292573bdc92a9c7a8e470e33f8c526f311
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8152
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Instead of the two different representations (which we don't really
use much), use a `Box<str>` (which potentially shaves another 8 bytes
off `Value`).
NixString values themselves are immutable anyways (which was a
guarantee we already had with `SmolStr`), so this doesn't change
anything else.
Change-Id: I1d8454c056c21ecb0aebc473cfb3ae06cd70dbb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8151
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
The size of a `Vector<Value>` is 64 *bytes*, which is quite large, and
it bloated the entire Value type to this size.
This change adds an indirection for the inner vector through Rc.
Initially I tried to use a Box, but this breaks pointer equality
guarantees for the Vector when it is small enough to be inlined.
This reduces the size of Value from 64 to 32 bytes.
Change-Id: Ic3211e861b1966c78b2c3d536ba291fea92647fd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8150
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
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
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>
These functions will be used by the changes in the VM to observe the
runtime execution of generator frames, and provide a more linear view
of the execution of the Tvix VM.
Change-Id: I10b1b1933dedc065e7c61d5d6062f0aaeee0097e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8240
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
In order to implement an asynchronous builtins.sort (required for
moving builtins to generators), we need an `async` sorting algorithm
as our comparators involve invoking a Nix function.
This commit implements a fairly simple, optimised bubble sort as the
sorting algorithm used in our `async fn sort_by`.
There don't seem to be any crates providing async versions of things
like this, and they might actually be pretty hard to implement
generically due to some constraints about how `async` works.
Note that this algorithm is less efficient than the hybrid
"timsort/mergesort/insert sort" used in the Rust standard library. I
tried to write a merge sort implementation, but ran into isuses with
the sort becoming unstable because our comparators can not yield
equality. This is the simplest implementation which I know to be
correct.
Note that as of this commit this is *not* covered by the Tvix test
suite, but it will be as soon as the rest of the generator code lands.
Change-Id: Ia9a604f7dd941d6acc9212c902e0e637ed75bebc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8239
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
We currently send two warnings in case of detecting dead code - W008
inside compile_dead_code, and a more detailed warning in all places that
invoke compile_dead_code:
```
warning[W007]: useless operation on boolean: this expression is always false
--> /nix/store/qz3gjn95gazab4fkb7s8lm6hz17rdzzy-414z9nnj1wy66ymq6vgb693x9xjz6hf2-nixpkgs-src/pkgs/top-level/perl-packages.nix:12079:15
|
12079 | doCheck = false && !stdenv.isDarwin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^
warning[W008]: this code will never be executed
--> /nix/store/qz3gjn95gazab4fkb7s8lm6hz17rdzzy-414z9nnj1wy66ymq6vgb693x9xjz6hf2-nixpkgs-src/pkgs/top-level/perl-packages.nix:12079:24
|
12079 | doCheck = false && !stdenv.isDarwin;
| ^^^^^^^^^^^^^^^^
```
The place invoking `compile_dead_code` has more context to why the code
is unused, so it's error message is much more useful.
Stop emitting the less informative warning inside compile_dead_code
(W008), and update the comment that we expect the caller to emit a
warning.
I kept W008 itself still around, in case we end up having places this
will get used again.
Change-Id: I2c5d84fc0cb4035872cd4b71cc3e9e34e120eb37
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8024
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
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>
As applies are thunked, there was no situation where OpCall could be
emitted. In practice, all calls were already tail calls.
Change-Id: Id0d441dcdd86f804d7cddd0cc14f589bbfc75e5b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8147
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Because they do not use it, and it can not be passed with the coming
generator refactoring.
Change-Id: I0d96f2357a7ee79cd8a0f401583d4286230d4a6b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8146
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
