[ML] Adding memory usage methods for categorization classes #859
Conversation
This is a step towards adding memory instrumentation to categorization. On its own this PR has no externally visible effect, but by splitting out these boilerplate changes the PR that eventually adds memory instrumentation to categorization will be smaller.
edsavage
left a comment
edsavage
left a comment
There was a problem hiding this comment.
LGTM, just a few questions and comments
| using TMultiIndex = boost::multi_index::multi_index_container<T, I, A>; | ||
| constexpr std::size_t indexCount{ | ||
| boost::mpl::size<typename TMultiIndex::index_type_list>::value}; | ||
| std::string componentName(name); |
There was a problem hiding this comment.
could componentName be const std::string &?
There was a problem hiding this comment.
It gets modified 8 lines below.
There would be other ways to do what this method does, but I copied the structure from the adjacent boost::circular_buffer method. I think there's some benefit in keeping the same high level structure for these similar methods.
| ptr->setName(usage); | ||
|
|
||
| componentName += "_item"; | ||
| for (auto i = t.begin(); i != t.end(); ++i) { |
There was a problem hiding this comment.
The idiomatic way to iterate with iterators is auto rather than const auto&. All the equivalent loops in the file are done like this one. The auto variable is the iterator, not the container element.
But it did make me wonder why all these loops are using iterators rather than for (const auto& i : t) {. I tried changing them and got this warning:
/Users/dave/ml-cpp/include/core/CMemory.h:334:30: warning: loop variable 'i' is always a copy because the range of type 'const std::vector<bool>' does not return a reference [-Wrange-loop-analysis]
for (const auto& i : t) {
So I think it's because sometimes the vector method will have to deal with vector<bool>, so that needs the explicit iterator version to avoid the warning, and all the others are done the same way for consistency.
Obviously I could change the loops except the vector one, but I do think with this file that there's a benefit in keeping as much as possible consistent between the methods that deal with different types of container. There are already subtle differences just dealing with the container structure differences. I'm not sure it's good to introduce a difference in the way looping is done too.
There was a problem hiding this comment.
Weird.. Thanks for looking in to it!
| boost::mpl::size<typename TMultiIndex::index_type_list>::value}; | ||
| std::size_t mem = 0; | ||
| if (!memory_detail::SDynamicSizeAlwaysZero<T>::value()) { | ||
| for (auto i = t.begin(); i != t.end(); ++i) { |
lib/core/CompressUtils.cc
Outdated
| mem->setName("CCompressUtil"); | ||
| core::CMemoryDebug::dynamicSize("m_FullResult", m_FullResult, mem); | ||
| if (m_ZlibStrm.state != nullptr) { | ||
| mem->addItem("m_ZlibStrm", 5952); // See below for where 5952 came from |
There was a problem hiding this comment.
Could we maybe also use a named constant for the 5952 value?
This is a step towards adding memory instrumentation to categorization. On its own this PR has no externally visible effect, but by splitting out these boilerplate changes the PR that eventually adds memory instrumentation to categorization will be smaller. Backport of elastic#859
This is a step towards adding memory instrumentation to categorization. On its own this PR has no externally visible effect, but by splitting out these boilerplate changes the PR that eventually adds memory instrumentation to categorization will be smaller. Backport of #859
This was introduced by elastic#859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Relates elastic#922
This was introduced by #859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Relates #922
This was introduced by elastic#859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Backport of elastic#940
This was introduced by elastic#859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Backport of elastic#940
This was introduced by #859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Backport of #940
This was introduced by #859. A raw pointer with a confusing typedef name was assumed to be a smart pointer when it wasn't. Also checked for the same problem in other places, and didn't find any, but corrected a couple of local typedef names. (The wider problem that CMemoryUsage::TMemoryUsagePtr aliases a raw pointer will have to wait for a bigger PR.) Backport of #940
2 participants
This is a step towards adding memory instrumentation to
categorization. On its own this PR has no externally
visible effect, but by splitting out these boilerplate
changes the PR that eventually adds memory instrumentation
to categorization will be smaller.