| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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
|
|
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>
|
|
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>
|
|
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
|
|
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>
|
|
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
|
|
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>
|
|
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>
|
|
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>
|
|
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
|
|
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
|
|
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>
|
|
C++ Nix forces and typechecks the passed argument even if it is not
necessary in order to compute the return value of the function. I
discovered this when I thought our formals miscompilation might be that
we are too strict, but doesn't look like it in this case.
Change-Id: Ifb3c92592293052c489d1e3ae8c7c54e4b6b4dc6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8701
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
|
|
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 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>
|
|
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
|
|
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/D99517
https://github.com/WebAssembly/proposals/pull/157
https: //github.com/rust-lang/rfcs/issues/2691#issuecomment-1462152908
Change-Id: Id15945d5a92bf52c16d93456e3437f91d93bdc57
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8290
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
|
|
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
|
|
* 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
|
|
Change-Id: I93b485ddd280cc15fcbaecf4aed5fcd22e28a8a8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8212
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>
|
|
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
|
|
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
|
