mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(tvix/eval): drop LightSpan::Delayed

Browse source 
LightSpan::Delayed was introduced in commit
bf286a54bc 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 is contained in:
Adam Joseph 2023-12-08 22:33:12 -08:00 committed by clbot
parent 29878259b3
commit c92d06271d
2 changed files with 1 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
tvix/eval/src/spans.rs
View file

@ -1,12 +1,9 @@
//! Utilities for dealing with span tracking in the compiler and in
//! error reporting.
use crate::opcode::CodeIdx;
use crate::value::Lambda;
use codemap::{File, Span};
use rnix::ast;
use rowan::ast::AstNode;
use std::rc::Rc;
/// Helper struct to carry information required for making a span, but
/// without actually performing the (expensive) span lookup.
@ -19,17 +16,9 @@ pub enum LightSpan {
/// The span has already been computed and can just be used right
/// away.
Actual { span: Span },
/// The span needs to be computed from the provided data, but only
/// when it is required.
Delayed { lambda: Rc<Lambda>, offset: CodeIdx },
}
impl LightSpan {
pub fn new_delayed(lambda: Rc<Lambda>, offset: CodeIdx) -> Self {
Self::Delayed { lambda, offset }
}
pub fn new_actual(span: Span) -> Self {
Self::Actual { span }
}
@ -37,7 +26,6 @@ impl LightSpan {
pub fn span(&self) -> Span {
match self {
LightSpan::Actual { span } => *span,
LightSpan::Delayed { lambda, offset } => lambda.chunk.get_span(*offset),
}
}
}

2
tvix/eval/src/vm/mod.rs
View file

@ -161,7 +161,7 @@ impl CallFrame {
/// but without performing that calculation.
// TODO: why pub?
pub(crate) fn current_light_span(&self) -> LightSpan {
LightSpan::new_delayed(self.lambda.clone(), self.ip - 1)
LightSpan::new_actual(self.current_span())
}
}