Conversation
| delete[] ptr->trt_ep_context_file_path; | ||
| delete[] ptr->trt_onnx_model_folder_path; | ||
| if (ptr->trt_op_types_to_exclude) delete[] ptr->trt_op_types_to_exclude; | ||
| if (!ptr->trt_op_types_to_exclude && ptr->trt_op_types_to_exclude_str_is_dynamic_allocation) delete[] ptr->trt_op_types_to_exclude; |
There was a problem hiding this comment.
Because we allow user to provide empty string or nullptr to trt_op_types_to_exclude option to override default value.
In OrtApis::UpdateTensorRTProviderOptions, the value of trt_op_types_to_exclude can be '\0' or nullptr, and later it calls TensorrtExecutionProviderInfo::UpdateProviderOptions and when handling the trt_op_types_to_exclude options with copy_string_if_needed(), the trt_op_types_to_exclude is assigned with nullptr.
so, here in the ReleaseTensorRTProviderOptions, we need to check that pointer is not nullptr, otherwise deleting a nulltpr leads to SIGABRT.
| trt_provider_options_v2.trt_onnx_bytestream_size = internal_options.onnx_bytestream_size; | ||
| trt_provider_options_v2.trt_op_types_to_exclude = copy_string_if_needed(internal_options.op_types_to_exclude); | ||
| if (options.find("trt_op_types_to_exclude") != options.end()) { | ||
| trt_provider_options_v2.trt_op_types_to_exclude = copy_string_if_needed(internal_options.op_types_to_exclude); |
There was a problem hiding this comment.
copy_string_if_needed() doesn't always dynamically allocate memory right? only if string_copy is true?
this is_dynamic_allocation check looks ugly.
There was a problem hiding this comment.
i can add a check that only if string_copy is true, we set is_dynamic_allocation to true.
yeah, agree the is_dynamic_allocation is ugly. But i can't think of a better solution without having a flag.
The reason is options_v2.trt_op_types_to_exclude which is a const char* can be static allocation or dynamic allocation.
- static allocation - When instantiate an OrtTensorRTProviderOptionsV2 instance, the trt_op_types_to_exclude is static allocation.
- dynamic allocation - When user calls OrtApis::UpdateTensorRTProviderOptions and specify other value to trt_op_types_to_exclude, then it's dynamic allocation.
So, once ReleaseTensorRTProviderOptions is being called, it's hard to tell that trt_op_types_to_exclude is static allocation or dynamic allocation. (Note: We can't delete a static allocation buffer). That's why we might need a flag here.
There was a problem hiding this comment.
It's an internal file. Why don't we just use a std::string? Then there will be no need to manually delete[] buffers.
There was a problem hiding this comment.
We later discussed we will close this PR due to ugly handling of the buffers.
| if (options.find("trt_op_types_to_exclude") != options.end()) { | ||
| trt_provider_options_v2.trt_op_types_to_exclude = copy_string_if_needed(internal_options.op_types_to_exclude); | ||
| trt_provider_options_v2.trt_op_types_to_exclude_str_is_dynamic_allocation = 1; | ||
| } |
There was a problem hiding this comment.
what if we dynamically allocate the ""NonMaxSuppression,NonZero,RoiAlign" string in the else case here if we don't find trt_op_types_to_exclude in options. then we would always need to deallocate it.
There was a problem hiding this comment.
what if we dynamically allocate the ""NonMaxSuppression,NonZero,RoiAlign" string in the else case here if we don't find trt_op_types_to_exclude in options.
i assume you mean the default value "NonMaxSuppression,NonZero,RoiAlign" of trt_op_types_to_exclude, it's static allocation, no need to deallocate. it will be deallocated automatically by the program.
Otherwise, If user uses OrtApis::UpdateTensorRTProviderOptions to update the OrtTensorRTProviderOptionsV2 instance, i don't think the case you mention exist.
if user don't use our API and directly update trt_op_types_to_exclude in the OrtTensorRTProviderOptionsV2 instance, it's user's responsibility to deallocate the buffer.
sophies927
removed
triage:approved
4 participants
Fix wrong logic and mem leak