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

[Relay] Resolve cyclic dependency in Closure #4139

Closed
MarisaKirisame opened this issue Oct 17, 2019 · 4 comments
Closed

[Relay] Resolve cyclic dependency in Closure #4139

MarisaKirisame opened this issue Oct 17, 2019 · 4 comments

Comments

@MarisaKirisame
Copy link
Contributor

Right now relay leak memory for all of the executor (aot, interpreter, vm), for even the simplest program (id = fun x => x). They all happened to be caused by the exact same problem.
In Relay Implementation, a closure capture over all the free variables, holding a reference to all of them. However, it also hold a reference of itself (as long as it has a name, while ANF give everything a name). This make all closure cyclic data structure.
I propose to solve it by making the two following modification:
add a new Value, RecFunc. RecFunc hold a shared_ptr of a vector of closure, and a size_t index into it.
Every Closure will now have another field, a Map<Var, size_t> which store all recursive call (if any, including mutually recursive one) from the parent RecFunc.
This is in inspiration from https://cs.indiana.edu/ftp/techreports/TR73.pdf -
L2 )_)GS48MTGU8~RILJ{D8
I will submit a PR soon, issuing here for visibility so ppl can avoid the same problem again.

@tqchen tqchen changed the title [Relay] Memory Leak [Relay] Resolve cyclic dependency in Closure Oct 17, 2019
@tqchen
Copy link
Member

tqchen commented Oct 17, 2019

I am not sure I get it, as for recursive call we are capturing Variable, as opposed to the its binding I think this problem worth a bit of more thoughts. Essentially it was the same problem in the global modules, in which case we resolve by global var. Do you think if it is possible to do the same thing, eg lamda lift then only capture global vars. You will need the module map to be able to run closure

@tqchen
Copy link
Member

tqchen commented Oct 17, 2019

@MarisaKirisame given that it is an interesting design question, can you open an RFC discuss thread ?

@MarisaKirisame
Copy link
Contributor Author

let's take this standard program, the factorial, for example.
let fact = \x -> if x == 0 then 1 else x * fact (x - 1)
The closure object of x contain the value of all free variable of fact - in that case, also a shared_ptr reference to the fact closure too!
I will open an RFC later.

@tqchen
Copy link
Member

tqchen commented Oct 18, 2019

consolidate to #4143

@tqchen tqchen closed this as completed Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants