-
|
I have very simple code to create a TF_STRING tensor from a std::string and it leaks memory in TF_TString_Copy() (invoked from tensor.h) in tensorflow lib when exiting the scope. The Address Sanitizer reported the following leak. I am not sure how cppflow handles destruction when the tensor is no longer used. It seems the TF_TString needs some special handling in ~tensor()? Not sure if we need to call TF_TString_Dealloc(&tstr[0]) somewhere? |
Beta Was this translation helpful? Give feedback.
Replies: 7 comments 7 replies
-
|
Is the author of the cppflow able to look into the leak issue @serizba ? |
Beta Was this translation helpful? Give feedback.
-
|
my test with Asan doesn't complain about memory leak any more with this fix.
…On Tue, May 24, 2022 at 12:09 PM Jiannan Liu ***@***.***> wrote:
@ns-wxin <https://github.com/ns-wxin> Could you test PR #128
<#128> ?
—
Reply to this email directly, view it on GitHub
<#200 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWGZCH5JBJBKDKUFZIWBHKDVLUSNTANCNFSM5WUGOL2A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
However, what I need is a TF_String with shape {1}. Can you provide a tensor constructor that takes a vector of std::string? When I did the following, I got "heap-use-after-free". Maybe I'm not doing it right? I also tried the following and still got similar "heap-use-after-free". The tensor call is straight-forward: Here's Asan complaint about "heap-use-after-free" |
Beta Was this translation helpful? Give feedback.
-
|
If the fix is not working when one has to use the cppflow::tensor created, I guess we could try to release the memory after using the tensor. So somewhere in ~tensor()? I don't have a model that takes a TF_TString with shape {}, I only have one with TF_TString {-1} |
Beta Was this translation helpful? Give feedback.
-
|
I think the problem is that tstr is not moved into the tensor, instead the
pointers are copied. If you call TF_TString_Dealloc(&tstr[i]) in the ctor,
it causes use after free later.
With the current copy approach, tstr must outlive the tensor object, or
proper move and deallocation is needed.
…On Tue, May 24, 2022, 18:21 Wayne Xin ***@***.***> wrote:
If the fix is not working when one has to use the cppflow::tensor created,
I guess we could try to release the memory after using the tensor. So
somewhere in ~tensor()? I don't have a model that takes a TF_TString with
shape {}, I only have one with TF_TString {-1}
signature_def['serving_default']:
The given SavedModel SignatureDef contains the following input(s):
inputs['sentences'] tensor_info:
dtype: DT_STRING
shape: (-1)
name: serving_default_sentences:0
The given SavedModel SignatureDef contains the following output(s):
outputs['output_0'] tensor_info:
dtype: DT_FLOAT
shape: (-1, 50)
name: StatefulPartitionedCall_2:0
—
Reply to this email directly, view it on GitHub
<#200 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALDTHXSROE7HUSRSZSTAFTVLVI7ZANCNFSM5WUGOL2A>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
that's what I suspected. So maybe put the TF_TString_Dealloc() in the destructor of cpp::tensor. Also, if I call tensor.get_data<TF_TString>(), I wouldn't be able to do it due to type not mapped: code snippet: |
Beta Was this translation helpful? Give feedback.
-
|
I checked the problem with the TF_STRING. The solution in #128 does not work, as it deallocates before using the tensor. It seems that when resizing the string, then the extra buffer is not cared by TF_DeleteTensor. I think that a simple workaround would be to change the deallocator to also take into account the TF_STRING, like this: inline tensor::tensor(const std::string& value) {
auto deallocator = [](TF_Tensor* tft) {
TF_TString* tstr = static_cast<TF_TString*>(TF_TensorData(tft));
TF_DeleteTensor(tft);
TF_TString_Dealloc(&tstr[0]);
};
this->tf_tensor = {TF_AllocateTensor(static_cast<enum TF_DataType>(TF_STRING), nullptr, 0, sizeof(TF_TString)), deallocator};
TF_TString* tstr = static_cast<TF_TString*>(TF_TensorData(this->tf_tensor.get()));
TF_TString_Init(&tstr[0]);
TF_TString_Copy(&tstr[0], value.c_str(), value.length());
this->tfe_handle = {TFE_NewTensorHandle(this->tf_tensor.get(), context::get_status()), TFE_DeleteTensorHandle};
status_check(context::get_status());
}What do you guys think? For the multidimensional issue with @ns-wxin , with this solution it would be easy to create a tensor constructor that takes a |
Beta Was this translation helpful? Give feedback.
-
|
@serizba Thanks for the fix. I need some time to test it and maybe some unittests to verify. |
Beta Was this translation helpful? Give feedback.
-
|
Nice. I will wait to include this on the main code, cause I'm not quite sure about this hacky solution. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @ljn917 Did you test this solution? Should we include into the master code? |
Beta Was this translation helpful? Give feedback.
-
|
@serizba thanks for the fix. Yes. ASAN not longer complains about the memory leak. There is one small thing that I am not sure what happened. You can see my model by scrolling up that I take input ("sentence") of shape -1, which is supposed to be an array of DT_STRING. The constructor that takes a So with your recent fix, I had to do the following to get the right result. Basically I have to force the shape to be {1}. with this small tweak, I'm able to get the expected result of course without memory leak (thanks). The corresponding python code is as following. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @ns-wxin That's because the code that I posted here is meant to be used to create a non-dimensional (scalar) string. As you need a vector of strings (although of size 1), you should use the fix that I posted in your other issue #196. And call |
Beta Was this translation helpful? Give feedback.
-
|
Hi @serizba , Thank you for the info. I found a bug in my code in constructing the vectorstd::string. Yes, the #196 works as expected. Thank you. -Wayne |
Beta Was this translation helpful? Give feedback.
Hi @ns-wxin @ljn917
I checked the problem with the TF_STRING. The solution in #128 does not work, as it deallocates before using the tensor.
It seems that when resizing the string, then the extra buffer is not cared by TF_DeleteTensor. I think that a simple workaround would be to change the deallocator to also take into account the TF_STRING, like this: