impl/lola: Added shm-size calculation by analysis for DATA/CONTROL - #760
impl/lola: Added shm-size calculation by analysis for DATA/CONTROL#760crimson11 wants to merge 7 commits into
Conversation
c72a678 to
a090932
Compare
a090932 to
59cd98d
Compare
| enum class ShmSizeCalculationMode : std::uint8_t | ||
| { | ||
| kSimulation, | ||
| kEstimation, |
There was a problem hiding this comment.
This is no longer an estimation - this is now an calculation! We should be clear in the wording.
59cd98d to
e088dba
Compare
| /// \brief A fixed-capacity, associative container with a std::map-like interface backed by a contiguous array. | ||
| /// | ||
| /// \details LinearSearchMap replaces the previously used std::map / boost::interprocess::map inside the shared-memory | ||
| /// data structures (e.g. ServiceDataStorage). Those map-types allocate memory dynamically on each insertion which makes |
There was a problem hiding this comment.
| /// data structures (e.g. ServiceDataStorage). Those map-types allocate memory dynamically on each insertion which makes | |
| /// data structures (e.g. ServiceDataStorage). Those map-types allocate memory dynamically on each insertion (the exact amount depends on the implementation) which makes |
| template <typename Key, | ||
| typename MappedType, | ||
| typename KeyEqual = std::equal_to<Key>, | ||
| typename Allocator = memory::shared::PolymorphicOffsetPtrAllocator<std::pair<Key, MappedType>>> |
There was a problem hiding this comment.
Should have scoped_allocator_adaptor?
| return key_equal_; | ||
| } | ||
|
|
||
| mapped_type& at(const Key& key) |
There was a problem hiding this comment.
| mapped_type& at(const Key& key) | |
| mapped_type& at(const Key& key)& |
I think this should have the trailing & so that it can't be called on a temporary(would lead to a dangling reference).
There was a problem hiding this comment.
Hm. I'm not convinced ;) Why?
- we resemble a std::map interface here! map doesn't lvalue qualify at() neither. This confuses imho.
- then I would need to be consistent! Also would have to add it o plenty of other methods! Like begin()/end() etc. they are all returning iterators/pointers which may be dangling in case of a temporary ...
| /// \param max_number_of_elements maximum number of elements the map can hold (its fixed capacity). | ||
| /// \param resource memory resource used to allocate the underlying storage. | ||
| /// \param key_equal binary predicate used to compare keys for equality. | ||
| LinearSearchMap(const size_type max_number_of_elements, |
There was a problem hiding this comment.
IMO, the definition of these functions should all be below. The declaration of the class should ideally fit in one screen (or a little bit of scrolling). But as it is now, it's quite hard to read the interface of this class.
| { | ||
| } | ||
|
|
||
| iterator begin() noexcept |
There was a problem hiding this comment.
Shouldn't be noexcept (unless this is about be compliant with some standard container specification)
There was a problem hiding this comment.
At least map/unordered_map have noexcept spec ... so I took it over.
| storage_{nullptr}, | ||
| control_qm_{nullptr}, | ||
| control_asil_b_{nullptr}, | ||
| number_of_service_elements_{0U}, |
There was a problem hiding this comment.
This should be a std::optional to ensure that we don't access it before it's set in CreateSharedMemory
| return ShmResourceStorageSizes{control_data_size, control_qm_size, control_asil_b_size}; | ||
| } | ||
|
|
||
| namespace |
There was a problem hiding this comment.
The anonymous namespace should go at the top of the file (to match the rest of the code base)
| // alignof(std::max_align_t) we keep the running offset max-aligned and therefore obtain a size that is guaranteed to be | ||
| // sufficient (it is exact whenever the allocated sizes are multiples of the involved alignment, which is the common | ||
| // case). | ||
| constexpr std::size_t RoundUpToMaxAlign(const std::size_t size) noexcept |
There was a problem hiding this comment.
This is a duplicate of CalculateAlignedSize?
There was a problem hiding this comment.
Yes! Somewhat! I repaced with CalculateAlignedSize(size, kMaxAlignedSize)
| { | ||
| const auto field_it = lola_service_instance_deployment_.fields_.find(name); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(field_it != lola_service_instance_deployment_.fields_.cend(), | ||
| "Could not find field in deployment configuration."); |
There was a problem hiding this comment.
The message should not just be repeating what the check is in words. It should either be explaining why the assertion is checked / expected or just omit it. It's not adding any new information here. Same for the other assertions
| return total_size; | ||
| } | ||
|
|
||
| std::size_t SkeletonMemoryManager::GetNumberOfSampleSlotsFromConfig(const std::string_view service_element_name, |
There was a problem hiding this comment.
What about something like this to avoid code duplication?
template <ServiceElementType ServiceElementType>
std::size_t SkeletonMemoryManager::GetNumberOfSampleSlotsFromConfig(const std::string_view service_element_name) const
{
SCORE_LANGUAGE_FUTURECPP_PRECONDITION(ServiceElementType == ServiceElementType::EVENT ||
ServiceElementType == ServiceElementType::FIELD);
const auto& lola_service_element_deployment = [this, service_element_name]() {
if constexpr (ServiceElementType == ServiceElementType::EVENT)
{
return lola_service_instance_deployment_.events_.at(std::string{service_element_name});
}
else
{
return lola_service_instance_deployment_.fields_.at(std::string{service_element_name})
.lola_event_instance_deployment_;
}
}();
const std::string name{service_element_name};
const auto field_it = lola_service_element_deployment.fields_.find(name);
SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(field_it != lola_service_instance_deployment_.fields_.cend(),
"Could not find field in deployment configuration.");
const auto number_of_slots = field_it->second.lola_event_instance_deployment_.GetNumberOfSampleSlots();
SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(number_of_slots.has_value(),
"Number of sample slots not specified for field.");
return number_of_slots.value();
}
There was a problem hiding this comment.
Same for GetMaxSubscribersFromConfig
There was a problem hiding this comment.
Hm. I'm not convinced. Just compare the src-code sizes. It is not really much less source-code and the original was also "easy" to grasp imho. Now you are templating it and in reality we will have larger code-size, because we have now the impl. instantiated twice ... and as I said - the new impl. still has a cosnstexpr if-else, which makes the code not really simpler?
If you still insist -> I can change ;)
The type DataTypeMetaInfo, which we use on our public interface is too weak! It doesn't assure invariants on size/alignment, which C++ demands: size has to be always a power of two and be an integer multiple of tzhe alignment. This commit switches to internal use of memory::DataTypeSizeInfo which forces this. On the public interface we keep the weak DataTypeMetaInfo for now to avoid a breaking change! In the public layer we transform DataTypeMetaInfo to DataTypeSizeInfo. In case this isn't possible because DataTypeMetaInfo is invalid, we return an error in the public API.
Currently we determine shm-sizes of the CTRL and DATA shm-objects by a simulation run. I.e. we initialize the content of both section 1st within a heap-allocated resource. At the end we use the sizes to correctly size the shm-objects. This "SIMULATION" method is exact but has high runtime costs and might consume lots of memory during startup. This change now redesigns the containers/dynamic data types being used within the DATA section, to use only classes, which we are in control of and where we exactly know based on our configuration, who much size they will need. Thus the whole simulation canbe skipped and we calculate the size correctly from the configuration settins for the service instance. We re-introduce therefore the ESTIMATION mode in parallel to the SIMULATION mode.
Added analytical estimate/ANALYSIS mode for the shm-size calculation for the CONTROL section.
Adapted signature of LinearSearchMap to support custom KeqEqual just like std::unordered_map.
In the skeleton component tests we were testing the lola::Skeleton with a event and field service element. But we only provided the config/ deployment info for these elements, but did never register them at their parent Skeleton. Thus, essential tests checking the shm-size calculation were off! The tests now correctly register the elements.
Fixed review comments for ne ANALYSIS based size calculations. Added unit test for parsing of new ANALYSIS mode.
Moved the shm-size calc for ServiceDataStorage and ServiceDataControl out of SkeletonMemoryManager to the correspoinding data structures itself.
6c3c9cf to
4c24cbc
Compare
Currently we determine shm-sizes of the
CTRL and DATA shm-objects by a simulation run.
I.e. we initialize the content of both section
1st within a heap-allocated resource. At the end
we use the sizes to correctly size the shm-objects. This "SIMULATION" method is exact but has high runtime costs and might consume lots of memory during startup.
This change now redesigns the containers/dynamic data types being used within the DATA section, to use only classes, which we are in control of and where we exactly know based on our configuration, who much size they will need. Thus the whole simulation canbe skipped and we calculate the size correctly from the configuration settins for the service instance.
We re-introduce therefore the ESTIMATION mode in parallel to the SIMULATION mode.
Tackles issue #761