-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak in cb_explore_adf #1698
Fix memory leak in cb_explore_adf #1698
Conversation
@@ -581,6 +581,7 @@ void finish(cb_explore_adf& data) | |||
data.prepped_cs_labels[i].costs.delete_v(); | |||
data.prepped_cs_labels.delete_v(); | |||
data.gen_cs.pred_scores.costs.delete_v(); | |||
data.gen_cs.mtr_ec_seq.~vector(); |
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 object is created in cb_explore_adf.cc by calling
T* temp = calloc_or_throw<T>(1);
new (temp) T();
and we only called free
of this object in learner::finish()
method in learner.h
. We have to call destructor of each fields in this method. Followed the same pattern in line 570.
@JohnLangford @ataymano Alexey and I had a discussion that we may want to introduce destructors to struct in VW instead of having finish()
method explicitly calling the destructor in each of the fields. This might be a long term goal. Open to discuss.
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.
Good catch.
A destructor would prevent oversights of this sort.
Thinking about it though, maybe this could be done incrementally by creating a destructor and just using finish() to call the destructor?
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.
yes, that was the proposal for the first step.
I agree destructors would be great, the number of explicit destructors being called makes me nervous |
* Update vowpal_wabbit submodule * Pulls in these fixes - fac3c06 ("Fix crash when empty multi_ex is supplied for --cb_explore_adf (VowpalWabbit#1679)") - f085178 ("bugfix: cb_adf is not including some examples in stats calculation (VowpalWabbit#1686)") - c9d9af7 ("Fix memory leak in cb_explore_adf (VowpalWabbit#1698)") - 1e8742b ("Fix some VW initialization memory leaks (VowpalWabbit#1697)") - 75423e3 ("Fix best constant and best constant's loss calculation when using ksvm (VowpalWabbit#1704)") - 2b64c3c ("Use std::exp instead of exp free function (VowpalWabbit#1709)") - 6b7b160 ("Fix unused params warnings plus incomplete struct init (done to default values). (VowpalWabbit#1710)") - 2e1602d ("Compile fixes (VowpalWabbit#1713)") - b50fb59 ("Make cbify reduction respect is_learn parameter (VowpalWabbit#1722)") This also includes the option parsing changes - b8ab396 ("`options_i` command line parsing refactor (VowpalWabbit#1706)" - 5b020c4 ("Ataymano/mac options types fix (VowpalWabbit#1718)") There are other small or non-rl-consumable changes * Enable injection of msbuild path in Windows command-line build * Update vowpal_wabbit submodule * This pulls in the additional fix made to address the bug discovered in the previous attempt to update this. Fixed in VowpalWabbit#1742 * Updates safe_vw.cc to get rid of a no-longer-necessary cleanup step
No description provided.