Skip to content

Fix run info memleaks #412

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 17, 2020
Merged

Fix run info memleaks #412

merged 5 commits into from
Jun 17, 2020

Conversation

lantiga
Copy link
Contributor

@lantiga lantiga commented Jun 16, 2020

Addresses #411

filipecosta90 and others added 2 commits June 16, 2020 18:27
@filipecosta90 filipecosta90 self-requested a review June 16, 2020 23:59
@filipecosta90
Copy link
Collaborator

@lantiga one of the leaks is solved, but we're still not freeing the tensors from the local context:

==11626== 602,341 (32 direct, 602,309 indirect) bytes in 1 blocks are definitely lost in loss record 119,347 of 119,347
==11626==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11626==    by 0x67CA99A: AI_dictExpand (dict.c:198)
==11626==    by 0x67CC27B: _dictExpandIfNeeded (dict.c:967)
==11626==    by 0x67CC362: _dictKeyIndex (dict.c:1009)
==11626==    by 0x67CAE34: AI_dictAddRaw (dict.c:341)
==11626==    by 0x67CAD75: AI_dictAdd (dict.c:306)
==11626==    by 0x67E3AFB: RedisAI_DagRunSession (dag.c:47)
==11626==    by 0x67DB41B: RedisAI_Run_ThreadMain (background_workers.c:186)
==11626==    by 0x53E56DA: start_thread (pthread_create.c:463)
==11626==    by 0x571E88E: clone (clone.S:95)

@filipecosta90
[fix] fixed reference count on ai.dagrun and ai.dagrunro for tensor s…
9e060bb 
…tructure. Added AI_dictType AI_dictTypeTensorVals with proper valDestructor
@filipecosta90 filipecosta90 linked an issue Jun 17, 2020 that may be closed by this pull request
memleak on AI.DAGRUN -- RedisAI_RunInfo and RAI_ModelRunCtx data structures are not being fully freed #411
Closed
@filipecosta90
Copy link
Collaborator

@lantiga the commit 9e060bb addresses the previous leak and adds the tests to it.
Can you check it?
It also adds AI_dictType AI_dictTypeTensorVals with proper valDestructor so that we don't forget to free values on dicts.

@filipecosta90 filipecosta90 requested a review from DvirDukhan June 17, 2020 01:53
filipecosta90
filipecosta90 approved these changes Jun 17, 2020
View reviewed changes
Copy link
Collaborator

@filipecosta90 filipecosta90 left a comment

Choose a reason for hiding this comment

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

LGTM for now. I believe it should be merged and cherry-picked to 1.0. cc @K-Jo

@lantiga
Copy link
Contributor Author

lantiga commented Jun 17, 2020

Hi @filipecosta90, thanks for the further fix, I think we're good now.

@lantiga lantiga merged commit ccf0d5c into master Jun 17, 2020
@lantiga lantiga deleted the rinfo_leaks branch June 17, 2020 07:28
@lantiga lantiga mentioned this pull request Jun 17, 2020
Closed
lantiga added a commit that referenced this pull request Jun 26, 2020
@lantiga @filipecosta90
Fix run info memleaks (#412)
39990f7 
* [add] added mobilenet model use case to quickly check for leaks on modelruns with large tensors (dagrun and modelrun variations)

* Fix run info memleaks

* [fix] fixed reference count on ai.dagrun and ai.dagrunro for tensor structure. Added AI_dictType AI_dictTypeTensorVals with proper valDestructor

* [fix] removed WIP unit test folder (not for this issue)

Co-authored-by: filipecosta90 <[email protected]>
filipecosta90 added a commit that referenced this pull request Jun 27, 2020
@lantiga @filipecosta90
Cherry pick on 1.0 (#419)
d0bbc92 
* Fix run info memleaks (#412)

* [add] added mobilenet model use case to quickly check for leaks on modelruns with large tensors (dagrun and modelrun variations)

* Fix run info memleaks

* [fix] fixed reference count on ai.dagrun and ai.dagrunro for tensor structure. Added AI_dictType AI_dictTypeTensorVals with proper valDestructor

* [fix] removed WIP unit test folder (not for this issue)

Co-authored-by: filipecosta90 <[email protected]>

* Add test for inconsistent zero batch output

* Add missing files

* YOLOv3 crash with empty image hotfix

Co-authored-by: filipecosta90 <[email protected]>
This was referenced Aug 20, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipecosta90 filipecosta90 filipecosta90 approved these changes

@DvirDukhan DvirDukhan Awaiting requested review from DvirDukhan

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

memleak on AI.DAGRUN -- RedisAI_RunInfo and RAI_ModelRunCtx data structures are not being fully freed
2 participants
@lantiga @filipecosta90