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

feat(tvix/eval): Leak strings (with flag to disable)

Browse source 
Default to always leaking strings, and copying strings by copying
pointers rather than cloning the underlying allocation. This (somewhat
bafflingly) doesn't seem to affect any benchmarks, but paves the way for
some tricks around string allocation that do.

Unfortunately, we can't do this (yet?) for contextful strings, for
reasons I don't currently understand but which I will address later,
when I address contextful strings more holistically.

I've left a flag in here to disable this, both to test the cloning logic
for strings for when/if we decide to bring this back, and to allow
people who care more about memory usage than perf to disable leaking.

Change-Id: Iec44bcbfe9b3d20389d2450b9a551792a79b9b26
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12045
Autosubmit: aspen <root@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This commit is contained in:
Aspen Smith 2024-07-28 11:37:50 -04:00 committed by clbot
parent 480a8106cf
commit 59056cf705
3 changed files with 21 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
tvix/Cargo.nix
View file

@ -16122,7 +16122,7 @@ rec {
"proptest" = [ "dep:proptest" ]; "proptest" = [ "dep:proptest" ];
"test-strategy" = [ "dep:test-strategy" ]; "test-strategy" = [ "dep:test-strategy" ];
}; };
resolvedDefaultFeatures = [ "arbitrary" "default" "impure" "nix_tests" "proptest" "test-strategy" ]; resolvedDefaultFeatures = [ "arbitrary" "default" "impure" "nix_tests" "no_leak" "proptest" "test-strategy" ];
}; };
"tvix-eval-builtin-macros" = rec { "tvix-eval-builtin-macros" = rec {
crateName = "tvix-eval-builtin-macros"; crateName = "tvix-eval-builtin-macros";

5
tvix/eval/Cargo.toml
View file

@ -58,6 +58,11 @@ impure = []
# Enables Arbitrary impls for internal types (required to run tests) # Enables Arbitrary impls for internal types (required to run tests)
arbitrary = ["proptest", "test-strategy", "imbl/proptest"] arbitrary = ["proptest", "test-strategy", "imbl/proptest"]
# Don't leak strings (enable this if you care about peak memory usage of eval)
#
# This is intended as a stop-gap until we have a garbage collector
no_leak = []
[[bench]] [[bench]]
name = "eval" name = "eval"
harness = false harness = false

18
tvix/eval/src/value/string.rs
View file

@ -409,6 +409,10 @@ unsafe impl Sync for NixString {}
impl Drop for NixString { impl Drop for NixString {
fn drop(&mut self) { fn drop(&mut self) {
if !cfg!(feature = "no_leak") {
return;
}
// SAFETY: There's no way to construct a NixString that doesn't leave the allocation correct // SAFETY: There's no way to construct a NixString that doesn't leave the allocation correct
// according to the rules of dealloc // according to the rules of dealloc
unsafe { unsafe {
@ -419,9 +423,17 @@ impl Drop for NixString {
impl Clone for NixString { impl Clone for NixString {
fn clone(&self) -> Self { fn clone(&self) -> Self {
// SAFETY: There's no way to construct a NixString that doesn't leave the allocation correct if cfg!(feature = "no_leak") || self.context().is_some() {
// according to the rules of clone // SAFETY: There's no way to construct a NixString that doesn't leave the allocation correct
unsafe { Self(NixStringInner::clone(self.0)) } // according to the rules of clone
unsafe { Self(NixStringInner::clone(self.0)) }
} else {
// SAFETY:
//
// - NixStrings are never mutated
// - NixStrings are never freed
Self(self.0)
}
} }
} }