-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix memory leak reported by ASAN in NNVM to ONNX conversion #15516
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR @kevinzh92 I agree with the fix. May I suggest to change this to a vector of float and rename the variable to a more semantic name?
@larroy isn't it consistent with the existing naming system? |
Not really, in those cases you want a semantic name, even if you are wrapping your Cat in a pointer you want to name it:
And not
Semantic naming is important, we have already type information from the compiler and we are in a statically typed language, no need to name a pointer _ptr. And definitely not "data" for the name of a variable, this is a code smell. |
Gotcha! Makes sense! |
@kevinzh92 Could you please address the review comments? |
eea6687
to
5c6383d
Compare
Sorry guys. Things have been busy at work and this fell off my radar. @larroy, I hope I addressed all of your concerns. |
c419ce4
to
856fe16
Compare
initializer_proto->add_float_data(data_ptr[blob_idx]); | ||
} | ||
std::vector<float> constants(size); | ||
nd.SyncCopyToCPU(&constants, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙅♂️this is not correct. You are taking address to the vector itself, you want to take address to the data.
Please use .data() member or the address of the first element.
856fe16
to
f3c2276
Compare
f3c2276
to
3957383
Compare
3957383
to
326fe0b
Compare
Looks like there's still a compiler issue:
|
I also have concerns about this line:
|
Hi, I update this code to use Thank @kevinzh92 for pointing out this issue. |
Please rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merged. Thank you! |
Description
Fix memory leak reported by ASAN in NNVM to ONNX conversion of a constant.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
shared_ptr
, as written, does not calldelete[]
on a dynamically allocated array.Comments