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

SameDiff.reverseArrayLookup causes severe memory leak #5934

Closed
emillynge opened this issue Jul 19, 2018 · 7 comments

Comments

@emillynge
Copy link

commented Jul 19, 2018

SameDiff uses a IndentityHasMap to resolve which SDVariable is bound to a given INDArray. This however, creates a permanent reference to the array which is never purged and hence, the arrays cannot be deallocated.

When training a network using the new and fancy SameDiffLayer (Hooray! such awesome), this causes a very fast buildup of off heap memory resulting in inevitable OOM exception.

I have tried using a WeakHashMap locally and that seems to have fixed the problem.
Since none of the concrete implementations of INDArray overrides Object.hashCode(), this should be safe for now. But ideally we would use a WeakIdentityHashMAp.

@emillynge

This comment has been minimized.

Copy link
Author

commented Jul 20, 2018

@AlexDBlack I would think this is an easy fix that some of the more frequent committer could just add to an existing PR. But I can also file a PR myself if preferable.
But I would need to know whether we just want to use WeakHashMap and hope no one has overridden hashCode method in some weird subclass. Or if we want to implement/pull in dependency on WeakIdentityHashMap

@saudet

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

I was thinking a simpler solution is just to remove the arrays from the IdentityHashMap whenever we update the other maps holding the INDArrays.
Though I can't think of a lot of cases where this functionality is used in practice, so maybe we can remove the IdentityHashMap outright.

@AlexDBlack AlexDBlack self-assigned this Jul 21, 2018

AlexDBlack added a commit that referenced this issue Jul 21, 2018
@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

I ended up removing it here: #5940
It's not really used anywhere.
I'll close this issue once that PR is merged.

AlexDBlack added a commit that referenced this issue Jul 21, 2018
ND4J/SameDiff Fixes (#5940)
* #5934 Remove memory leak - reverseArrayLookup identity hash map

* Tweak TF import test precision to avoid suprious failure on one test

* #5684 Fix SameDiff mmul 'result transpose'
@emillynge

This comment has been minimized.

Copy link
Author

commented Jul 21, 2018

Thank for the quick fix! @AlexDBlack

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

No problem - thanks for flagging the issue :)

@lock

This comment has been minimized.

Copy link

commented Sep 21, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Sep 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.