Skip to content

Conversation

@alonre24
Copy link
Collaborator

@alonre24 alonre24 commented Dec 29, 2020

  • In DAG_OP init, we allocate inkeys and outkeys arrays. When we parse model run command, we allocate inkeys and outkeys arrays as well (what causes the leaks). This PR contains a temporary fix, this will be refactor soon.
  • In DAG_commandParser, the following line causes a leak:
    RedisModuleString *mangled_key = RedisModule_CreateStringFromString(NULL, key);
    A call for RedisModule_FreeString was added.
  • The allocation of DAG_device_ops array in RunInfo_Init is redundant (it is initialized in every shallow copy).
  • The function Tensor_DataTypeStr allocates memory on the heap to store the string that represents the tensor type, and it wasn't freed in one place. This allocation is replaced with stack allocation for char array - thus we save allocations and get rid of the leaks .

@alonre24 alonre24 self-assigned this Dec 29, 2020
@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #549 (ec613bf) into master (896799d) will increase coverage by 0.14%.
The diff coverage is 92.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   76.01%   76.16%   +0.14%     
==========================================
  Files          25       25              
  Lines        5508     5433      -75     
==========================================
- Hits         4187     4138      -49     
+ Misses       1321     1295      -26     
Impacted Files Coverage Δ
src/DAG/dag.c 86.88% <ø> (-0.27%) ⬇️
src/backends/tensorflow.c 67.16% <0.00%> (-0.21%) ⬇️
src/err.c 97.56% <ø> (+2.21%) ⬆️
src/tensor.c 83.57% <85.71%> (+0.82%) ⬆️
src/command_parser.c 90.00% <87.50%> (-0.63%) ⬇️
src/DAG/dag_parser.c 87.64% <100.00%> (+0.14%) ⬆️
src/background_workers.c 87.30% <100.00%> (-0.44%) ⬇️
src/model.c 70.38% <100.00%> (+0.83%) ⬆️
src/run_info.c 73.14% <100.00%> (+3.19%) ⬆️
src/script.c 65.65% <100.00%> (+0.83%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 896799d...ec613bf. Read the comment docs.

@lantiga
Copy link
Contributor

lantiga commented Dec 29, 2020

Hi @alonre24 , can you provide a small description of the changes and what the leak was due to?

lantiga
lantiga previously approved these changes Dec 30, 2020
Copy link
Contributor

@lantiga lantiga left a comment

Choose a reason for hiding this comment

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

LGTM, good catches

…to queue item(s) (even if the DAG is not finished), since we reinsert the item(s) to the queue (and reallocate memory).
@alonre24 alonre24 merged commit 64001de into master Dec 31, 2020
@alonre24 alonre24 deleted the fix_valgrind_leaks branch December 31, 2020 10:15
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.

4 participants