Skip to content

Conversation

DvirDukhan
Copy link

@DvirDukhan DvirDukhan commented Jan 4, 2021

  • tflite
  • torch
  • tf
  • onnx
  • common
  • dag
  • serializations

@DvirDukhan DvirDukhan marked this pull request as ready for review January 12, 2021 17:06
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #553 (340435d) into master (8243e8d) will decrease coverage by 1.96%.
The diff coverage is 86.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   74.53%   72.57%   -1.97%     
==========================================
  Files          35       35              
  Lines        5648     5626      -22     
==========================================
- Hits         4210     4083     -127     
- Misses       1438     1543     +105     
Impacted Files Coverage Δ
src/err.c 97.36% <ø> (-0.20%) ⬇️
src/serialization/AOF/rai_aof_rewrite.c 0.00% <ø> (ø)
src/libtflite_c/tflite_c.cpp 58.98% <50.00%> (+2.00%) ⬆️
src/backends/torch.c 69.49% <66.66%> (-11.20%) ⬇️
src/backends/tensorflow.c 68.64% <75.67%> (+1.47%) ⬆️
src/DAG/dag.c 82.86% <85.71%> (-2.95%) ⬇️
src/background_workers.c 80.55% <88.11%> (-6.76%) ⬇️
src/redisai.c 83.33% <94.44%> (+0.17%) ⬆️
src/backends.c 64.25% <100.00%> (+1.44%) ⬆️
src/backends/tflite.c 68.51% <100.00%> (ø)
... and 12 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 8243e8d...340435d. Read the comment docs.

src/run_info.c Outdated
long long ref_count = *rinfo->dagRefCount;
/* In case of client disconnect, this function will be called for cleanup.
* It needs to validate the execution has finished. */
while(ref_count > 0) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be atomic?

src/DAG/dag.c Outdated
for (size_t outputNumber = 0; outputNumber < noutputs; outputNumber++) {
RAI_Tensor *tensor = RAI_ScriptRunCtxOutputTensor(op->sctx, outputNumber);
tensor = tensor ? RAI_TensorGetShallowCopy(tensor) : NULL;
// tensor = tensor ? RAI_TensorGetShallowCopy(tensor) : NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment?

src/DAG/dag.c Outdated
}

RAI_FreeRunInfo(rinfo);
// RAI_FreeRunInfo(rinfo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment?

src/DAG/dag.c Outdated
RedisAI_RunInfo *rinfo = RedisModule_GetBlockedClientPrivateData(ctx);
RAI_FreeRunInfo(rinfo);
void RunInfo_FreeData(RedisModuleCtx *ctx, void *rinfo) {
RAI_FreeRunInfo(rinfo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The RunInfo_FreeData is called after the reply function ends?

// Block the client before adding rinfo to the run queues (sync call).
rinfo->OnFinish = DAG_ReplyAndUnblock;
rinfo->client = RedisModule_BlockClient(ctx, RedisAI_DagRun_Reply, NULL, RunInfo_FreeData, 0);
RedisModule_SetDisconnectCallback(rinfo->client, RedisAI_Disconnected);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't it necessary anymore?

@DvirDukhan DvirDukhan merged commit bd9f462 into master Jan 13, 2021
@DvirDukhan DvirDukhan deleted the fix_memory_leaks branch January 13, 2021 10:00
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.

2 participants