facebook / starlark-rust

A Rust implementation of the Starlark language
Apache License 2.0
672 stars 53 forks source link

Values passed to eval_function are never freed? #97

Open Youx opened 9 months ago

Youx commented 9 months ago

Hi,

I am trying to use eval_function() to call Starlark functions from Rust, as per the doc.

However, it seems that as long as the module is loaded, the values allocated on the heap passed to eval_function are never freed? The heap keeps growing indefinitely.

I have tried adding verbose_gc() to see if the garbage collector is run, but it shows nothing.

At first I thought it was an issue with my custom type definition, but it seems to behave the same with a standard rust type (ex: heap.alloc("qwe".to_string()).

I tried running it through a different evaluator each time (thinking each evaluator would have its own heap, but that's not the case).

Is there a way to call a function without leaking, except reloading the AST into a new module each time?

Best regards

Unrelated question : what is freeze all about? There are references to it everywhere in the doc, but no explanation about what it entails, when one should use it, ...

use starlark::{
    environment::{Globals, Module},
    eval::Evaluator,
    starlark_simple_value,
    syntax::{AstModule, Dialect},
    values::{starlark_value, Coerce, Freeze, NoSerialize, ProvidesStaticType, StarlarkValue},
};

starlark_simple_value!(T);
#[derive(
    Default,
    Coerce,
    ProvidesStaticType,
    NoSerialize,
    Debug,
    allocative::Allocative,
    starlark::values::Trace,
    Freeze,
    derive_more::Display,
)]
#[repr(C)]
struct T {
    a: i64,
}

#[starlark_value(type = "T")]
impl<'v> StarlarkValue<'v> for T {}

fn main() {
    let script = r#"
def x(a):
    pass
x
"#;
    let ast = AstModule::parse("x.star", script.to_owned(), &Dialect::Extended).unwrap();
    let globals = Globals::standard();
    let module = Module::new();
    let mut eval = Evaluator::new(&module);
    eval.verbose_gc();
    let _ = eval.eval_module(ast, &globals).unwrap();
    loop {
        let x = module.get("x").unwrap();
        let heap = eval.heap();
        let _ = eval
            // .eval_function(x, &[heap.alloc(T::default())], &[])
            //.eval_function(x, &[heap.alloc(5)], &[])
            .eval_function(x, &[heap.alloc("qwe".to_string())], &[])
            .unwrap();
        println!("Allocated: {}", heap.allocated_bytes());
    }
}
Youx commented 9 months ago

Ok, I found a way to work around this by freezing the original module (probably unnecessary?), and creating a temporary module each loop, in which the function from the frozen module is run.

cjhopman commented 6 months ago

That sounds like it's probably the right solution, it's fairly similar to how buck uses starlark evaluation. I don't think starlark modules are really designed to be long-lived, instead they are designed to be evaluated and then frozen, and the frozen form may be long-lived.

I don't know that there's a fundamental limitation that requires that, just that the use cases have fit well into the evaluate and freeze model. It may be that the interpreter could change to handle well having a long lived active module.

ndmitchell commented 6 months ago

Your solution is probably the right one. The idea is a Module is both the definition and some scratch space for temporary usage. You can garbage collect that with .garbage_collect() on an evaluator, but that requires there are no other Value's floating around.

https://github.com/facebookexperimental/starlark-rust/blob/main/docs/heaps.md and https://github.com/facebookexperimental/starlark-rust/blob/main/docs/values.md are useful to read. Leaving this ticket open though, since the second doc is outdated (with the right concepts) and the first is more mechanics. We probably need to combine them to make it user friendly, and fix everything outdated.