tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Memory leak with long-running nickel process #1908

Open jneem opened 2 months ago

jneem commented 2 months ago

Evaluation of recursive records can create Rc loops, in which a thunk's environment pointer can point to an environment that itself references the thunk. This creates a memory leak, which has been observed in nls background eval.

For one-shot eval (like in nickel export) this isn't a problem, but it might be a blocker for people trying to embed nickel.

yannham commented 2 months ago

One possibility would be to experiment with drop-in replacement of Rc with cycle detection, such as dumpster or a similar crates. We would have to watch the impact on performance, though. A great solution would be to be able to choose, but I don't know how easy it is, given the lack of HKT in Rust: I suspect we want to be able to be parametric in T<U> where T : * -> *, but this isn't representable. Although we have SharedTerm, ie maybe we only care about Rc<RichTerm> (and not for a generic parameter T) in which case we can abstract over it. Doing so, we could for example use the version with garbage collection for the REPL and the LSP, but keep the main evaluator performant by not dropping cyclic objects.