ponylang / ponyc

Pony is an open-source, actor-model, capabilities-secure, high performance programming language
http://www.ponylang.io
BSD 2-Clause "Simplified" License
5.69k stars 410 forks source link

Memory leak when val parameter implements iso parameter trait. #4102

Open SeanTAllen opened 2 years ago

SeanTAllen commented 2 years ago

The following program will leak memory, but can be switched to not leak memory as noted in the comments:

use "time"

actor Main
  new create(env: Env) =>
    Timers()(Timer(Notify(ReceiverConcrete), 20_000_000, 20_000_000))

class Notify is TimerNotify
  let _receiver: Receiver
  new iso create(receiver: Receiver) => _receiver = receiver
  fun ref apply(timer: Timer, count: U64): Bool =>
    _receiver.receive(recover iso Array[U8].init(0, 400_000) end)
    true

trait tag Receiver
  be receive(string: Array[U8] iso) // change this to val for stable memory

actor ReceiverConcrete is Receiver
  be receive(string: Array[U8] val) => None // OR change this to iso for stable memory

It should be noted that the "doesnt leak memory version" takes a while to reach "memory equilibrium" so it memory growth slowly occurs. The test program will rapidly leak memory with the "bad" version and you should see over a gig in memory used within a minute of running whereas the "good" version will have significantly less usage (it will stabilize at less than 100 megs within a few minutes).

The issue stems from the call going through a trait that is iso and traced as such and the receiving end being a val and then we don't do the correct tracing and objects end up not being freed.

In Zulip @jemc stated:

I can confirm that from a LLVM IR perspective, we will call pony_traceknown with PONY_TRACE_MUTABLE when the trait parameter type is iso, even when the concrete type's parameter type is val (and thus, PONY_TRACE_IMMUTABLE would be the correct trace kind to use)

I think probably the best way to solve this would be to change the selector painting / virtual tables around in such a way that we end up with a different selector color / virtual table index / message id for the val case vs the iso case

Even though they are compatible in the type system, they are not compatible at runtime and need to be treated separately.

Full zulip thread is at: https://ponylang.zulipchat.com/#narrow/stream/189985-beginner-help/topic/Debugging.20excessive.20memory.20usage

SeanTAllen commented 2 years ago

@jemc is going to add a write-up of the agreed upon (by he and I) solution.

SeanTAllen commented 1 year ago

@jemc did you ever do a write up? i don't remember our agreed upon solution.

SeanTAllen commented 1 year ago

Note that the fix for #1118 as seen in https://github.com/ponylang/ponyc/pull/4247 fixes this leak.

SeanTAllen commented 1 year ago

A full "non hack" fix for #1118 might not fix this leak.

jemc commented 1 year ago

Yes, I suspect that as we iteratively bring back the optimization removed in #4247 this memory leak will likely come back as well - I think it likely still needs a separate fix.

jemc commented 1 year ago

In fact, the minimal example in the ticket description should come back in the very first iteration of bringing back the optimization, as Array[U8] very obviously cannot contain an actor.