Skip to content

[CONTRIB] Add PopenWorker process recycling#11094

Merged
AndrewZhaoLuo merged 2 commits intoapache:mainfrom
petersalas:popen-pool-recycle
Apr 22, 2022
Merged

[CONTRIB] Add PopenWorker process recycling#11094
AndrewZhaoLuo merged 2 commits intoapache:mainfrom
petersalas:popen-pool-recycle

Conversation

@petersalas
Copy link
Contributor

Background processes (e.g. builds during autotuning) can sometimes accumulate state or leak memory. This adds the ability for PopenWorker processes to optionally be recycled after a specified number of uses.

Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I think this is a parsimonious (but good) workaround to the issue of memory leaks in some uses of PopenWorker. It would be cool if we could open an issue with an example B) if you have time.

@AndrewZhaoLuo
Copy link
Contributor

Wonder what others think though? @shingjan ?

@zxybazh
Copy link
Member

zxybazh commented Apr 22, 2022

Very interesting, we observe the same behaviour with PopenWorker during tuning and our workaround is also very similar. Would be happy to learn about how you guys figured out the problem and do you have any trace on the root cause or potential fix. A minimum reproducible example would also be appreciated!

Copy link

@shingjan shingjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending in this fix! Memory leak around PopenPool has been an issue identified in #11074. Does this fix imply that PopenExecutor underlying process will be a zombie process if used too many times? If so what is the best heuristic number for max_number_use?

@junrushao
Copy link
Member

junrushao commented Apr 22, 2022

That's exactly one issue that we recently identified and fixed in #11074. Thank you so much for this fix! Also CC @tqchen as the original author of PopenWorker

@AndrewZhaoLuo AndrewZhaoLuo marked this pull request as ready for review April 22, 2022 04:39
@AndrewZhaoLuo
Copy link
Contributor

Do we have an existing issue for the memory leak stuff? I think based on some internal data we suspect it's in tvm.lower somewhere.

@zxybazh
Copy link
Member

zxybazh commented Apr 22, 2022

Just created one #11096.

@petersalas
Copy link
Contributor Author

@shingjan to be clear this change alone isn't enough to mitigate the leaks because the default is "never recycle" unless operations timeout. But specifying a value when initializing PopenPoolExecutor ensures that the background processes are killed periodically which prevents any per-operation leaks from accumulating too much. I used 32 in my test, which worked fine.

Of course, we should also try to fix the leaks :)

Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any objections? Otherwise will merge by eod

@junrushao
Copy link
Member

junrushao commented Apr 22, 2022

LGTM too! BTW, if maximum_uses is set, while the process is run async (e.g. MetaSchedule RPCRunner), we might want some logic to make sure synchronization happens before recycling the child processes, so that running results are not lost, which is beyond the scope of this PR

@AndrewZhaoLuo AndrewZhaoLuo merged commit 8691cbe into apache:main Apr 22, 2022
@junrushao
Copy link
Member

Thanks @petersalas for bringing this workaround, and @AndrewZhaoLuo @shingjan @zxybazh for review and discussion :-))

shtinsa pushed a commit to Deelvin/tvm that referenced this pull request May 17, 2022
* [CONTRIB] Add PopenWorker process recycling

* Clarify docstrings

Co-authored-by: Peter Salas <psalas@octoml.ai>
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

Successfully merging this pull request may close these issues.

5 participants