Avoid manual memory management ImageSeriesReader::m_MetaDataDictionaryArray#5280
Conversation
Declared `ImageSeriesReader::m_MetaDataDictionaryArray` private, just like `ImageSeriesWriter::m_MetaDataDictionaryArray`. Following C++ Core Guidelines, Oct 3, 2024, "Avoid `protected` data", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-protected Note that the dictionaries of `ImageSeriesReader` remain publicly accessible by `GetMetaDataDictionaryArray()`.
blowekamp
left a comment
blowekamp
left a comment
There was a problem hiding this comment.
If I am not mistaken this introduces extra copies to the dictionary.
| EncapsulateMetaData<double>(newDictionary, "ITK_non_uniform_sampling_deviation", spacingDeviation); | ||
| } | ||
| m_MetaDataDictionaryArray.push_back(newDictionary); | ||
| m_InternalMetaDataDictionaries.push_back(newDictionary); |
There was a problem hiding this comment.
Copying the dictionary is a non-trivial task. The prior implementation used pointer to ensure extra copying was not done. This now needs to manually be done.
This should be "emplace_back" to reduce copying the dictionary. Also when the vector is resize, it will copy the dictionaries. The vector should reserve the expected number of dict ( aka number of slices.
There was a problem hiding this comment.
@blowekamp Thanks for expressing your concern, but I think you are mistaken. Copying a dictionary is actually pretty fast, now that it has "copy on write". For more than six years already:
- Look at pull request Meta data dictionary move and copy on write #455 commit 327de92 (especially look at the author 😺)!
No problem, I can add a vector::reserve call, and a std::move, but I don't expect a major performance change 🤷
There was a problem hiding this comment.
@blowekamp Please check if your concern is taken away by this force-push
There was a problem hiding this comment.
Ahh... Thanks for the link, lol. And thank you for using the move semantics which is fine, I agree it will not make much difference given the "brilliant" implement in the DataDictionary 🤣
Side details discussion: I do find your C++ changes interesting and frequently learn things from them. Thank you. I suppose resizing and vector where the objects support move semantics is not too expensive, even without lazy copying.
No longer manually allocated MetaDataDictionary objects on the heap. Stored them in an internal `std::vector` instead.
N-Dekker
force-pushed
the
Style-m_MetaDataDictionaryArray
branch
from
b128652 to
d7150cd
Compare
| m_MetaDataDictionaryArray.push_back(newDictionary); | ||
| m_InternalMetaDataDictionaries.push_back(std::move(newDictionary)); |
There was a problem hiding this comment.
@blowekamp Regarding your suggestion to use "emplace_back": in this particular case I think emplace_back and push_back are basically equivalent, technically. emplace_back(newDictionary) would have copied newDictionary, albeit "in-place" ("emplaced"). emplace_back(std::move(newDictionary)) would have moved newDictionary. But the current push_back(std::move(newDictionary)) also just moves newDictionary.
In general, emplace_back is more interesting as an alternative to push_back(MyClass(a, b, c)), to replace it with emplace_back(a, b, c). (See also clang-tidy use-emplace.)
I don't think the guideline is to always replace push_back with emplace_back. 🤷
There was a problem hiding this comment.
I think the usage of emplace_back would have been to remove the local variable... something like:
m_InternalMetaDataDictionaries.emplace_back(reader->GetImageIO()->GetMetaDataDictionary());
My thought had been to use emplace_back to avoid extra copies of the objects being created. For this particular object it doesn't master. And your implementation is nearly equivalent.
There was a problem hiding this comment.
I think the usage of emplace_back would have been to remove the local variable... something like:
m_InternalMetaDataDictionaries.emplace_back(reader->GetImageIO()->GetMetaDataDictionary());
OK, but then it still wouldn't be much different from using push_back, as follows:
m_InternalMetaDataDictionaries.push_back(reader->GetImageIO()->GetMetaDataDictionary());
In either way (whether "emplacing" or "pushing"), it would always have to make a copy of the dictionary from the ImageIO object. 🤷 Fortunately copying a dictionary is very fast now, because of Copy On Write (your PR #455).
3 participants
No longer manually allocated the MetaDataDictionary objects of
ImageSeriesReaderon the heap. Stored them in an internalstd::vectorinstead.Declared
ImageSeriesReader::m_MetaDataDictionaryArrayprivate, just likeImageSeriesWriter::m_MetaDataDictionaryArray.