| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
* 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
|
|
Change-Id: I3dc30cca33fbbd8e8686655635ee471f5937d9f8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8257
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
|
|
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>
|
|
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>
|
|
... 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
|
|
Change-Id: I93b485ddd280cc15fcbaecf4aed5fcd22e28a8a8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8212
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>
|
|
Change-Id: I1282c3387ac1e0d1528b894814f2a495ca5a6a32
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8199
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
|
|
Change-Id: Ief20544a44c3542fe40a5c09f81d0f064a346f44
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8149
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
|
|
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>
|
|
Instead of using a suspended native thunk, calculate and optionally
insert the storeDir builtin when the VM is constructed.
We already have the IO handle available at this point and can just
check whether a storeDir is present, and insert its absolute value as
a builtin.
Change-Id: If966eee6ff26dc888b6e888e7c46170c0c346b05
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8145
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
|
|
This is a ThunkSet wrapped to be shareable, which will be required
once ThunkSets are embedded in futures.
Change-Id: I5a067b7972ac86e4d354c75ef05c86b2284c1137
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8144
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
|
|
Change-Id: Iea248870a0ea5d38cb02ff059c968fbd563570b6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8143
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
|
|
Again simplifying some code down the line, where bits of code that
construct attribute sets already have the final structure available.
Change-Id: I0bb7a1daa63298122b51be73d35d695a4f73f8b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8140
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
|
|
This simplifies some code down the line.
Change-Id: I58dd71e796e11479f44516cf24932f8061843d23
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8139
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
|
|
Change-Id: I4c02f0104c455ac00a3f299c1fbf75cbb08e8972
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8142
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
|
|
Change-Id: Ifb59ef148ea4fab613f2e4efb133c04baafa3a98
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8141
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
|
|
This adds addresses of thunk and closure chunks to the debug output
displayed when dumping bytecode.
This makes it possible to see in the dump which thunks are referenced
by constants in other thunks.
Change-Id: I2c98de5227e7cb415666cd3134c947a56979dc80
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8137
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
|
|
This CL removes redundant clone from value which is
going to be dropped without further use.
Change-Id: Ibd2a724853c5cfbf8ca40bf0b3adf0fab89b9be5
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8125
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
|
|
Change-Id: Ib991d68724a73886a8343d7f785b5b3aedd637ed
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8103
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
|
|
We want the address that the Rc is pointing to, not the address of the
Rc.
Change-Id: I8eba21677f242bbe4166c74d4aa4269c316076e3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8045
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
|
|
This CL address clippy warning which expects to
use `writeln` instead of `write` for strings with
new line.
Change-Id: Ia72a07502c60cfd489ecf1e3833b9d42d44a8b17
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8030
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
|
|
This CL address clippy warning about finding the zero
length of something using `is_empty()` instead of `len() == 0`.
Change-Id: I2b36c7c7b65b733609fc0dcd33be06f9d772bc9b
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8029
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
|
|
This branch was missing, and an assumption elsewhere just executed the
returned (broken) bytecode.
This fixes b/253.
Change-Id: I015023ba921bc08ea03882167f1f560feca25e50
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8090
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
|
|
These should be inspectable by callers.
Change-Id: Ia9ef871aa63958d06066aaea61b2aecbd217369b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8089
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
|
|
Plain paths like `foo/bar.nix` are also allowed, so we can not
determine this based on the prefix.
The upstream PR that is referenced in a comment here has a
significantly different interface than we expected, so I'm not
touching that comment yet in this CL before I've had more time to
digest it.
Change-Id: Iea33bbb35de9c00a7d7fedf64d02253c75c1cc9e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8032
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: Alyssa Ross <hi@alyssa.is>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
|
|
We only use Rc in `impl EvalIO for StdIO`, which is only included when
building with the "impure" feature.
Change-Id: Id29d647c899cbfcdda11abfb9fabd5aa7e24299f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8025
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
|
|
This reduces the size of `Builtin` from 88 (!) bytes to 8, and as the
largest variant of `Value`, the size of that type from 96 to 64.
The next largest type is NixList, clocking in at 64 bytes.
This has noticeable performance impact. In an implementation without
disk I/O, evaluating nixpkgs.stdenv looks like this:
Benchmark 1: tvix -E '(import <nixpkgs> {}).stdenv.drvPath'
Time (mean ± σ): 1.151 s ± 0.003 s [User: 1.041 s, System: 0.109 s]
Range (min … max): 1.147 s … 1.155 s 10 runs
After this change, it looks like this:
Benchmark 1: tvix -E '(import <nixpkgs> {}).stdenv.drvPath'
Time (mean ± σ): 1.046 s ± 0.004 s [User: 0.954 s, System: 0.092 s]
Range (min … max): 1.041 s … 1.053 s 10 runs
Change-Id: I5ab7cc02a9a450c0227daf1f1f72966358311ebb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8027
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
|
|
When resolving a select expression (`attrs.name` or `attrs.name or
default`), if the set compiles to a constant attribute set (as is most
notably the case with `builtins`) we can backtrack and replace that
attribute set directly with the compiled value.
For something like `builtins.length`, this will directly emit an
`OpConstant` that leaves the `length` builtin on the stack.
Change-Id: I639654e065a06e8cfcbcacb528c6da7ec9e513ee
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7957
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
|
|
This fixes a very complicated bug (b/246). Evaluation
progresses *much* further after this, leading to several less
complicated bugs likely being uncovered by this
What was the problem?
=====================
Previously, when evaluating a thunk, we had a code path that looked
like this:
match *thunk {
ThunkRepr::Evaluated(Value::Thunk(ref inner_thunk)) => {
let inner_repr = inner_thunk.0.borrow().clone();
drop(thunk);
self.0.replace(inner_repr);
}
/* ... */
}
This code path created a copy of the inner `ThunkRepr` of a nested
thunk, and moved that copy into the `ThunkRepr` of the parent.
The effect of this was that the original `ThunkRepr` (unforced!) lived
on in the original thunk, without the memoization of the subsequent
forcing applying to it.
This had the result that Tvix would repeatedly evaluate these thunks
without ever memoizing them, if they occured repeatedly as shared
inner thunks. Most notably, this would *always* occur when
builtins.import was used.
What's the solution?
====================
I have completely rewritten `Thunk::force_trampoline_self` to make all
flows that can occur in it explicit. I have also removed the outer
loop inside of that function, and resorted to more use of trampolining
instead.
The function is now well-commented and it should be possible to read
it from top-to-bottom and get a general sense of what is going on,
though the trampolining itself (which is implemented in the VM) needs
to be at least partially understood for this.
What's the new problem(s)?
==========================
One new (known) problem is that we have to construct `Error` instances
in all error types here, but we do not have spans available in some
thunk-related situations. Due to b/238 we cannot ask the VM for an
arbitrary span from the callsite leading to the force. This means that
there are now code paths where, under certain conditions, causing an
evaluation error during thunk forcing will panic.
To fix this we will need to investigate and fix b/238, and/or add a
span tracking mechanism to thunks themselves.
What other impacts does this have?
==================================
With this commit, eval of nixpkgs mostly succeeds (things like stdenv
evaluate to the same hashes for us and C++ Nix, meaning we now
construct identical derivations without eval breaking).
Due to this we progress much further into nixpkgs, which lets us
uncover more additional bugs. For example, after this commit we can
quickly see that cl/7949 introduces some kind of behavioural issue and
should not be merged as-is (this was not apparent before).
Additionally, tvix-eval is now seemingly very fast. When doing
performance analysis of a nixpkgs eval, we now mostly see the code
path for shelling out to C++ Nix to add things to the store in there.
We still need those code paths, so we can not (yet) do a performance
analysis beyond that.
Change-Id: I738525bad8bc5ede5d8c737f023b14b8f4160612
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8012
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
|
