rheem-ecosystem / rheem

Rheem - a cross-platform data processing system
https://rheem-ecosystem.github.io
5 stars 0 forks source link

Resource leak in loops #34

Closed sekruse closed 7 years ago

sekruse commented 7 years ago

There seems to be a resource leak associated with loops: Rheem uses a reference counting scheme to manage its resources, in particular persisted ChannelInstances. Now, if a ChannelInstance inside of a loop is propagated to a subsequent loop iteration (because it is needed in the next iteration or because it tells whether to stop the loop), then it seems that there is a ExecutionStageLoopIterationContexts that obtains a reference to the ChannelInstance without releasing it.

luckyasser commented 7 years ago

@Muhaeb That probably explains your memory leak problem ?

sekruse commented 7 years ago

I think, I found the bug. We use "transition" contexts in between two iterations. It seems that we have not been declaring the erasure of the transition contexts properly to the reference counting facility. Thus, all "transition" channel instances were not swept, either.

@Muhaeb It would be great to hear if that eliminates or at least reduces said memory leak problem.

Muhaeb commented 7 years ago

So is it fixed now, can i sync and test?

On 28 Mar 2017 6:37 p.m., "Sebastian Kruse" notifications@github.com wrote:

I think, I found the bug. We use "transition" contexts in between two iterations. It seems that we have not been declaring the erasure of the transition contexts properly to the reference counting facility. Thus, all "transition" channel instances were not swept, either.

@Muhaeb https://github.com/Muhaeb It would be great to hear if that eliminates or at least reduces said memory leak problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rheem-ecosystem/rheem/issues/34#issuecomment-289810793, or mute the thread https://github.com/notifications/unsubscribe-auth/AE4GH87UG6XaKubbelsLeLYLQDqJmqjXks5rqSlTgaJpZM4MrxW3 .

sekruse commented 7 years ago

I have a fix, but I need to run the tests first and merge the fix if it passes. I will close this issue as soon as I am done with this.

sekruse commented 7 years ago

Okay, it's merged to the master.

Muhaeb commented 7 years ago

Lovely, thanks a million for spotting that and providing the fix.

On 28 Mar 2017 6:44 p.m., "Sebastian Kruse" notifications@github.com wrote:

I have a fix, but I need to run the tests first and merge the fix if it passes. I will close this issue as soon as I am done with this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rheem-ecosystem/rheem/issues/34#issuecomment-289812698, or mute the thread https://github.com/notifications/unsubscribe-auth/AE4GH4AvTVOLdEFOxX0OD1SgNAI1zxhdks5rqSrAgaJpZM4MrxW3 .