fix memleak

[ivanallen]

Fix memory leak.

[ljn917]

Class defer seems to be duplicating the function of std::unique_ptr. I don't know how @serizba feels about it.

[ivanallen]

Class defer seems to be duplicating the function of std::unique_ptr. I don't know how @serizba feels about it.

Emm... It's not really the same thing. defer is more general.

[ljn917]

I am mostly neutral (though slightly negative) on the defer class here, because it can be implemented using std::unique_ptr without too much complication. However, I like defer as a general cleanup as in golang.

…

On Sat, Nov 7, 2020, 03:00 Allen ***@***.***> wrote: Class defer seems to be duplicating the function of std::unique_ptr. I don't know how @serizba <https://github.com/serizba> feels about it. Emm... It's not really the same thing. defer is more general. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALDTHWKZDK4GO6SQK75NEDSOT5A3ANCNFSM4TNPMYCA> .

[serizba]

@ivanallen @ljn917

I do not have a strong opinion on the topic either. I will accept the request if you both agree on it. If you think that just using unique_ptr may work, that would be perhaps cleaner.

[ljn917]

@ivanallen I am sorry that I didn't get much time to test this, my apologies. The other span PR may take some time too...

@serizba I think this is good to go. Although gcc's leak sanitizer still shows leaks in libcuda.so, I think it is false positive. cuda-memcheck --leak-check full ./example did not show any leak with the load_model example. I attached the leaks reported by gcc's leak sanitizer at the end to avoid further confusion.

If you still prefer std::unique_ptr over defer. We can define std::vecotr<std::unique_ptr<TF_Tenosr, decltype(&TF_DeleteTensor)>> inp_val_ptr;, then emplace_back each elements of inp_val into inp_val_ptr as @ivanallen suggested. Again, both solutions are acceptable in my opinion.

=================================================================
==685408==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 896 byte(s) in 16 object(s) allocated from:
    #0 0x7f60ce44c587 in __interceptor_calloc (/lib64/libasan.so.6+0xab587)
    #1 0x7f60a8f0a128  (/lib64/libcuda.so.1+0x1c8128)

Indirect leak of 1536 byte(s) in 16 object(s) allocated from:
    #0 0x7f60ce44c587 in __interceptor_calloc (/lib64/libasan.so.6+0xab587)
    #1 0x7f60a8f0a14a  (/lib64/libcuda.so.1+0x1c814a)

Indirect leak of 345 byte(s) in 16 object(s) allocated from:
    #0 0x7f60ce44c3cf in __interceptor_malloc (/lib64/libasan.so.6+0xab3cf)
    #1 0x7f60a90b99dd  (/lib64/libcuda.so.1+0x3779dd)

Indirect leak of 300 byte(s) in 15 object(s) allocated from:
    #0 0x7f60ce44c3cf in __interceptor_malloc (/lib64/libasan.so.6+0xab3cf)
    #1 0x7f60a901d654  (/lib64/libcuda.so.1+0x2db654)

Indirect leak of 83 byte(s) in 4 object(s) allocated from:
    #0 0x7f60ce44c3cf in __interceptor_malloc (/lib64/libasan.so.6+0xab3cf)
    #1 0x7f60a901d984  (/lib64/libcuda.so.1+0x2db984)
=================================================================

[ivanallen]

Thanks, @ljn917 @serizba!

I list the difference between defer and unique_ptr. Both solutions are okay for me, but we need to reach a consensus.

• use defer

    std::vector<TF_Tensor*> inp_val(inputs.size());

+   defer d([&inp_val]{
+       for (auto* tf_tensor : inp_val) TF_DeleteTensor(tf_tensor);
+   });

• use unique_ptr

+   std::vecotr<std::unique_ptr<TF_Tenosr, decltype(&TF_DeleteTensor)>> inp_val_ptr;
    std::vector<TF_Tensor*> inp_val(inputs.size());

    //...
    for (...) {
       // ...
       auto inp_tensor = TFE_TensorHandleResolve(std::get<1>(inputs[i]).tfe_handle.get(), context::get_status());
+      inp_val_ptr.emplace_back(inp_tensor, TF_DeleteTensor);
+      inp_val[i] = inp_tensor; // copy raw pointer to anther vector
    }

   //...

[ljn917]

I think @serizba should be the person to make the final decision here :)

[serizba]

Thanks for showing both options @ivanallen,

Now I think that perhaps using defer is cleaner. It adds a nice way of cleaning an object, and the class may be convenient in other future occasions too. Having a duplicate vector just to store the deleters seems a bit confusing.

[ivanallen]

New problems will be resolved in a new pull request.