From c92d06271d30394ec09ae3046efe4b671a7e266f Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Fri, 8 Dec 2023 22:33:12 -0800 Subject: feat(tvix/eval): drop LightSpan::Delayed LightSpan::Delayed was introduced in commit bf286a54bc2ac5eeb78c3d5c5ae66e9af24d74d4 which claimed that "This reduces the eval time for `builtins.length (builtins.attrNames (import {}))` 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 Autosubmit: Adam Joseph Tested-by: BuildkiteCI --- tvix/eval/src/spans.rs | 12 ------------ tvix/eval/src/vm/mod.rs | 2 +- 2 files changed, 1 insertion(+), 13 deletions(-) (limited to 'tvix') diff --git a/tvix/eval/src/spans.rs b/tvix/eval/src/spans.rs index c0130a665428..f422093b0d12 100644 --- a/tvix/eval/src/spans.rs +++ b/tvix/eval/src/spans.rs @@ -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, offset: CodeIdx }, } impl LightSpan { - pub fn new_delayed(lambda: Rc, 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), } } } diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 99a913c46ac9..e11b3b5d13c4 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -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()) } } -- cgit 1.4.1