Skip to content
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

[6.x][ML] Convert std::shared_ptrs to std::unique_ptrs where possible #111

Closed
wants to merge 1 commit into from

Conversation

tveasey
Copy link
Contributor

@tveasey tveasey commented May 30, 2018

Backport #108.

…stic#108)

Many of our current uses of shared pointer date from when not all our target platforms were
C++11 compliant and in particular move semantics weren't available. Now that they all are, we can
switch these to std::unique_ptr. This has a few advantages: i) it saves us 16 bytes (8 for the extra
pointer to the reference count and 8 for the reference count) per instance, ii) it avoids atomic
updates of the reference count, iii) it forces one to have the correct copy semantics in such
cases. For example, this showed up that we had an implicit shallow copy (not appropriate) on our
naive Bayes implementation and fixing this showed up a bug in restore. This also fixes an error in
memory accounting for shared pointers, which wasn't including the extra memory for the
reference count.
@droberts195
Copy link
Contributor

Please do not merge this until the problem of elastic/elasticsearch#30982 is resolved.

@tveasey
Copy link
Contributor Author

tveasey commented May 31, 2018

I backed this change out of master and am preparing a fixed version.

@tveasey tveasey closed this May 31, 2018
@sophiec20 sophiec20 added :ml and removed :ml labels Jun 12, 2018
@tveasey tveasey deleted the port/108 branch December 13, 2018 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants