Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Values passed to eval_function are never freed? #97

Open
Youx opened this issue Oct 23, 2023 · 3 comments
Open

Values passed to eval_function are never freed? #97

Youx opened this issue Oct 23, 2023 · 3 comments
Labels
documentation Improvements or additions to documentation

Comments

@Youx
Copy link

Youx commented Oct 23, 2023

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
Copy link
Author

Youx commented Oct 23, 2023

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
Copy link
Contributor

cjhopman commented Jan 3, 2024

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
Copy link
Contributor

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.

@ndmitchell ndmitchell added the documentation Improvements or additions to documentation label Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants