Add GetLatestSlot and GetLatestSample functionality by muhseth · Pull Request #208 · eclipse-score/communication

muhseth · 2026-03-18T12:59:58Z

No description provided.

LittleHuba · 2026-03-24T09:27:10Z

@muhseth please fix conflicts.

crimson11 · 2026-03-26T09:32:33Z

 }

+template <template <class> class AtomicIndirectorType>
+score::cpp::optional<SlotIndexType> EventDataControlCompositeImpl<AtomicIndirectorType>::FindLatestReadableSlotIndex(


You should instead change existing EventDataControlCompositeImpl::GetLatestTimestamp()! You are mostly duplicating code here. The new/refactored function should be named GetLatestSlot and it should return the index PLUS the timestamp!

And please take into account, what I have written here: Ticket-253464

Done.
I have removed GetLatestTimeStamp function and corresponding upper layer code.
As I see it is not used any where.

crimson11 · 2026-03-26T09:43:35Z

+         slot_index < static_cast<SlotIndexType>(control.state_slots_.size());
+         ++slot_index)
+    {
+        SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(static_cast<std::size_t>(slot_index) < control.state_slots_.size());


? Why this assert? I do not get it.

I have removed it. As I see it there is no needed of it.
I have kept it as it was the part of previous code.

SampleAllocateePtr changed because the mock binding variant has been changed to also carry a custom deleter. Reflect this change by reusing the infrastructure of SamplePtr, whose mock variant uses the same pattern. Original author: darkwisebear

crimson11 · 2026-04-08T13:04:22Z

    CheckForValidDataControls();
 }

+template <template <class> class AtomicIndirectorType>


You have to be aware, that you have to align your changes with Brenbdans massive change here: #268

I.e. imho it makes sense, that you base your work on Brendans PR!

crimson11 · 2026-04-08T13:40:13Z

 }

+template <template <class> class AtomicIndirectorType>
+score::cpp::optional<SlotIndexType> EventDataControlComposite<AtomicIndirectorType>::FindLatestReadableSlotIndex(


What does "readable" mean?
Just: FindNewestSlot ?

I think what Wahaj means is "FindLatestReferencedSlotIndex". I think it makes sense because just "newest" could also mean newest available slot to allocate in which isn't what we want.

crimson11 · 2026-04-08T13:51:57Z

 auto EventSlotStatus::IsInvalid() const noexcept -> bool
 {
-    return data_ == INVALID_EVENT;
+    return data_ == static_cast<value_type>(InvalidTimestamp);


This cast looks "odd" to me!
Our EventSlotStatus::value_type is an uint64 ... the timestamp is a uint32!
An invalid event-slot status technically means:
The timestamp part is 0 (upper32 bit)
the ref-count part is 0 (lower 32 bit)
Just casting the timestamp part does miss this a bit ... if you want to be more "expressive" you could write something like:
constexpr EventSlotStatus::value_type INVALID_EVENT = (InvalidTimestamp <<32) | ZeroRefCount)

but this is "overdone" -> you could simply document INVALID_EVENT = 0u means an invalid timestamp and zero refcount ...

i have made the changes as suggested.

crimson11 · 2026-04-08T16:47:17Z

 auto EventSlotStatus::MarkInvalid() noexcept -> void
 {
-    data_ = INVALID_EVENT;
+    data_ = static_cast<value_type>(InvalidTimestamp);


crimson11 · 2026-04-08T16:49:04Z

    static constexpr EventTimeStamp TIMESTAMP_MAX = std::numeric_limits<EventTimeStamp>::max();

+    /// \brief Timestamp value indicating that a slot is invalid / was never written.
+    static constexpr EventTimeStamp InvalidTimestamp{0U};


I'm fine with introducing these constexpr ... but be symmetric in naming? constexpr TIMESTAMP_MAX is capital letters ... so please no mixup

Does invalid really make sense here? Wouldn't something like UNINITIALIZED_TIMESTAMP make more sense?

Shouldn't this be private? If a user wants an uninitialized slot, shouldn't they simply default construct the EventSlotStatus?

crimson11 · 2026-04-08T16:55:35Z

    std::optional<EventDataControlComposite<>>&
-        event_data_control_composite_ref_;                    // Reference to the optional in derived class
-    EventSlotStatus::EventTimeStamp& current_timestamp_ref_;  // Reference to the timestamp in derived class
+        event_data_control_composite_ref_;  // Reference to the optional in derived class


This identation looks weird? .. and please fix the comment! lola::SkeletonEvent is NOT derived from SkeletonEventCommon! It aggregates it....
-> // Reference to the optional in the enclosing/aggregator class

Also, nitpick, but this isn't really a reference to an optional but rather an optional that is either empty or contains a reference? IMO, we can simply use a raw pointer here to avoid having to have std::optional<std::reference_wrapper>.

crimson11 · 2026-04-08T17:07:10Z

 }

+template <typename SampleType>
+Result<SampleType> SkeletonEvent<SampleType>::GetLatestSample() noexcept


Ouch. Why are you returning the sample by value here??? We never do this. A sample can be several megabytes in size ... so why are you copying it here - all our zero-copy architecture goes through the roof then ... and applications may crash, because the value will end up on stack, which will overflow ...

I guess you will use/need this functionality, when implementing the Skeleton-side get() method? Or even/also the set-method, where you need to work on the latest existing sample ...?

But then you need to apply SamplePtr logic!
I.e. you refcount the latest sample ... and create a SamplPtr from it, which you return from here ...
At your caller-side (your get/set impl) you then copy the data from the SamplePtr to your method ReturnType/InArg and once you are done with the copying, you destroy the SamplPtr freeing the slot.

This way you have just ONE copy! With your approach taken here, you copy TWICE! You copy out from shm to your sampl variable and then you copy from that variable back intro the call-queue! And this already expects, that you have RVO (which I guess is mandatory right now) ... but again - your caller side will crash, because the returned value (with RVO) will land most likely on stack and might explode there (if it is large).

crimson11 · 2026-04-08T17:24:18Z

+    {
+        const auto sample = event_data_storage_->at(static_cast<std::uint64_t>(slot.GetIndex()));
+
+        if (slot.IsValidQM())


OK - so here you are doing "manually", what should be the job of the SamplePtr dtor

bemerybmw · 2026-04-09T05:45:55Z

    static constexpr EventTimeStamp TIMESTAMP_MAX = std::numeric_limits<EventTimeStamp>::max();

+    /// \brief Timestamp value indicating that a slot is invalid / was never written.
+    static constexpr EventTimeStamp InvalidTimestamp{0U};


Does invalid really make sense here? Wouldn't something like UNINITIALIZED_TIMESTAMP make more sense?

bemerybmw · 2026-04-09T06:09:18Z

 /// the control block. It provides meta-information for an event. This class acts as an easy access towards this
 /// meta-information.
 ///
 /// \details This data structure needs to fit into std::atomic. Thus it's size shall not exceed the machine word size.


I know it's not part of this change, but can you add a static assert that sizeof(EventSlotStatus) <= sizeof(std::uint64_t)?

bemerybmw · 2026-04-09T06:19:13Z

+    static constexpr EventTimeStamp FirstValidTimestamp{1U};
+
    /// \brief If default constructed, SlotStatus is invalid
    EventSlotStatus() noexcept = default;


Should we dispatch to the other constructor with:

EventSlotStatus() noexcept : EventSlotStatus(UNINITIALIZED_TIMESTAMP) {}

It's more explicit and we can then remove the comment.

bemerybmw · 2026-04-09T06:21:19Z

    static constexpr EventTimeStamp TIMESTAMP_MAX = std::numeric_limits<EventTimeStamp>::max();

+    /// \brief Timestamp value indicating that a slot is invalid / was never written.
+    static constexpr EventTimeStamp InvalidTimestamp{0U};


Shouldn't this be private? If a user wants an uninitialized slot, shouldn't they simply default construct the EventSlotStatus?

bemerybmw · 2026-04-09T06:22:39Z

+    static constexpr EventTimeStamp InvalidTimestamp{0U};
+
+    /// \brief First valid timestamp value that can appear in shared memory.
+    static constexpr EventTimeStamp FirstValidTimestamp{1U};


Where is this actually used?

bemerybmw · 2026-04-09T06:41:00Z

+    const auto new_refcount = static_cast<EventSlotStatus::SubscriberCount>(slot_old.GetReferenceCount() + 1U);
+    EventSlotStatus slot_new{slot_old.GetTimeStamp(), new_refcount};
+
+    auto slot_old_value = static_cast<EventSlotStatus::value_type&>(slot_old);


Do you really want to cast to a reference? If yes, they should both be auto&. But I think that making a copy would probably be more performant than a reference anyway?

bemerybmw · 2026-04-09T06:43:05Z

+    EventSlotStatus slot_new{slot_old.GetTimeStamp(), new_refcount};
+
+    auto slot_old_value = static_cast<EventSlotStatus::value_type&>(slot_old);
+    auto slot_new_value = static_cast<EventSlotStatus::value_type&>(slot_new);


const auto slot_new_value

bemerybmw · 2026-04-09T06:46:58Z

+            auto& slot_qm = asil_qm_control_->state_slots_[slot_index];
+            auto& slot_asil_b = asil_b_control_->state_slots_[slot_index];
+
+            if (!TryIncreaseReferenceCount(slot_qm))


Why is this within the if (asil_b_control_ != nullptr)? Why not just do this first and then do the asil b one if needed? That way we don't need the duplicate call to if (!TryIncreaseReferenceCount(slot_qm)) below.

bemerybmw · 2026-04-09T06:50:57Z

+}
+
+template <template <class> class AtomicIndirectorType>
+bool EventDataControlComposite<AtomicIndirectorType>::TryIncreaseReferenceCount(ControlSlotType& slot) const noexcept


This function is missing recording transactions in the TransactionLog. We currently already have EventDataControl::ReferenceSpecificEvent? This function assumes that the slot stays in reading for the duration of the function (to be enforced by the caller). We can't make the same guarantee here because while this function is being called, the current slot could be overwritten be the provider. What about adding EventDataControl::TryReferenceSpecificEvent?

bemerybmw · 2026-04-09T07:01:08Z

+template <typename SampleType>
+Result<SampleType> SkeletonEvent<SampleType>::GetLatestSample() noexcept
+{
+    if (event_data_control_composite_.has_value() == false)


(!event_data_control_composite_.has_value())

This commit splits up ServiceDataControl, EventDataControl and EventControl into a data class (ServiceDataControl, EventDataControl and EventControl) which contains the data that is placed in shared memory and behaviour "view" classes (ServiceDataControlLocalView, EventDataControlLocalView and EventControlLocalView) which contain behaviour for interacting with the data via process local raw pointers which avoid performance overhead of interacting with data directly in shared memory (due to performance overhead of dealing with OffsetPtrs).

Since we always create a QM EventDataControl, we pass it into the composite by reference instead of by pointer to avoid doing nullptr checks within the composite (and for clearer semantics).

We previously introduced these classes to avoid additional bounds checking done when indexing into a DynamicArray in shared memory. Since we now are using the local views into shared memory, we no longer need this optimisation and can revert to simply accessing slots using their indices.

Since the static_cast can only return a nullptr if the usable base address is a nullptr, we assert that the base address is not a nullptr as this is better reflecting the intention of the assertion.

ReferenceSpecificEvent has two assertions and corresponding tests for these assertions which are very similar. Additional documentation is added to the assertions and the tests are refactored to make the distinction clearer.

These Impl classes were previously added to avoid having to change calling code to explicitly use the default template argument (i.e. EventSubscriptionControl<>). Using the explicit template argument makes the code more explicit and also removes the additional code complexity of having the alias and Impl class.

We remove noexcept from all signatures to allow us to use precondition violation tests instead of death tests. Precondition tests are orders of magnitude faster than death tests due to not having to fork a process and also don't have issues associated with RAII objects being destroyed twice (i.e. in forked process and main process).

We now return a struct instead of a pair. This makes it easier to add additional return types in future (we will soon return a reference to a TransactionLogSet as well) and also makes it clearer on caller side as they can access members using the names defined in the struct rather than .first / .second. We also return a reference to EventDataStorage instead of a pointer, since it should always be created.

Updated documentation of functions and general clean up of functions.

We use ScopeExit so that we can rely only the more complicated testing of move construction / move assignment from the ScopeExit class.

This change will be needed in an upcoming PR in which TransactionLogSet::Register() creates a TransactionLogRegistrationGuard. Both of thes files will need TransactionLogIndex so to avoid cyclic dependencies we extract TransactionLogIndex into a separate file.

Previously, we were returning an index and then manually creating a guard from the index. We now return the guard directly. This is important because in an upcoming PR, the guard will be responsible for unregistering the TransactionLog with the TransactionLogSet and also destroying the cached TransactionLogLocalView in the Proxy/SkeletonEventDataControlView. Therefore, we want to make the guard responsible for all cleanup.

This commit implements multiple related changes. To fully split these changes into atomic commits would require a huge amount of work in temporary refactoring in production code and updating a large number of unit tests in each commit. Much of these changes would be temporary and simply overwritten in the next commit. Therefore, we combine the changes into a single commit. * Moves TransactionLogSet from EventDataControl to EventControl TransactionLogSet is currently stored in EventDataControl. The ownership semantics here don't make sense: the EventDataControl is responsible for managing slot reference counting and part of this is to record transactions in the TransactionLogSet. However, we also need to record subscription transactions which occurs outside of EventDataControl. Therefore, we move TransactionLogSet into EventControl to make it clearer that it's not owned by EventDataControl and to avoid having to have getters in ProxyEventDataControlLocalView / SkeletonEventDataControlLocalView for the TransactionLogSet which makes no sense semantically. * Cache TransactionLogViews in ProxyEventDataControlLocalView ProxyEventDataControlLocalView is responsible for doing the reference counting in EventDataControl in shared memory. These operations must be recorded in a TransactionLog so that these operations can be undone when restarting in case the process containing a Proxy crashes. This TransactionLogLocalView is now cached inside the ProxyEventDataControlLocalView on ProxyEvent subscription (and removed on unsubscription) to avoid having to look up the view in the TransactionLogSet every time it's required. * TransactionLogRegistrationGuard now also injects the TransactionLogLocalView in ProxyEventDataControlLocalView on construction and clears it on destruction The TransactionLogRegistrationGuard manages the lifetime of a registered TransactionLog. As long as it's alive (it's alive as long as a ProxyEvent is subscribed), we want the cached TransactionLogLocalView to be stored in ProxyEventDataControlLocalView. Therefore, the TransactionLogRegistrationGuard is now also responsible for injecting / clearing this TransactionLogLocalView. * Since ProxyEventDataControlLocalView now contains a cached TransactionLogLocalView, we no longer need to pass the TransactionLogIndex around to lookup TransactionLogs in TransactionLogSet Getters for TransactionLogIndex are removed from ProxyEventCommon / SubscriptionStateMachine. Classes such as SlotCollector, SlotDecrementer etc. no longer receive a TransactionLogIndex as argument.

Dereferencing is something inherent to a consumer. Therefore, it should only be called by a ProxyEventDataControlLocalView. When a Skeleton creates or opens a shared memory region, it will only create a ProxyEventDataControlLocalView if tracing is enabled in the current process. However, when opening a shared memory region from a crashed process, it's possible that tracing was enabled in that process but currently disabled. Therefore, we may not have created a ProxyEventDataControlLocalView even though we need one to rollback old transactions. Therefore, the Skeleton creates a new ProxyEventDataControlLocalView just to perform the rollback.

The two local views currently are split based on behaviour. ProxyEventDataControlLocalView is responsible for all "consumer" behaviour e.g. referencing / dereferencing a slot and SkeletonEventDataControlLocalView is responsible for all "provider" behaviour e.g. allocating slots. Due to IPC tracing, we have a unique scenario in which a SkeletonEvent acts like a consumer (i.e. it creates a SamplePtr which it uses to keep a slot alive while the tracing daemon is using it). In this case, it needs to use the "consumer" behaviour. Since we can have consumer behaviour also on skeleton side, it doesn't make sense to call the class ProxyEventDataControlLocalView. Therefore we rename them to have Consumer / Provider prefixes.

- introduce InvalidTimestamp/FirstValidTimestamp constants - adapt timestamp tests

- moves common attributes to skeleton event common.

- Adds GetLatestSlot function to EventDataControlComposite

- Adds GetLatestSampleFunction to SkeletonEvent

bemerybmw · 2026-04-17T07:54:50Z

    ~SkeletonEventCommon() = default;

-    void PrepareOfferCommon(TransactionLogSet& transaction_log_set) noexcept;
+    void PrepareOfferCommon(TransactionLogSet& transaction_log_set, EventDataControlComposite<>& event_data_control_composite_ref) noexcept;


event_data_control_composite_ref should be passed by value?

bemerybmw · 2026-04-17T09:11:07Z

-    std::optional<EventDataControlComposite<>>&
-        event_data_control_composite_ref_;                    // Reference to the optional in derived class
-    EventSlotStatus::EventTimeStamp& current_timestamp_ref_;  // Reference to the timestamp in derived class
+    std::optional<EventDataControlComposite<>> event_data_control_composite_;


EventDataControlComposite should not contain a conumer event data control. (maybe don't to this now).

we should store here a qm and asil b consumer event data control local view.

qm is used for tracing and get (if the calling event is qm)

asil b is used for get (if the calling event is asil b)

TransactionLogs are registered by SkeletonEventCommon (qm or asil b consumereventdatacontrollocalviews are provided to register calls).

Move consumereventdatacontrollocalviews out of EventDataControlComposite. To be owned by SkeletonEventCommon and passed to SampleAllocateePtr directly.

Change Skeleton::Register to return EventDataControl& qm, EventDataControl* asil-b. Event now creates composite and ConsumerEventDataControlViews (which need EventDataControl).

muhseth requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners March 18, 2026 12:59

castler changed the title ~~Muse skeleton get~~ Add GetLatestSlot and GetLatestSample functionality Mar 19, 2026

LittleHuba marked this pull request as draft March 24, 2026 09:27

crimson11 requested changes Mar 26, 2026

View reviewed changes

muhseth force-pushed the muse_skeleton_get branch 3 times, most recently from c4ffb3a to f0c0b32 Compare April 2, 2026 09:58

muhseth marked this pull request as ready for review April 2, 2026 14:14

muhseth requested a review from crimson11 April 7, 2026 07:44

crimson11 requested changes Apr 8, 2026

View reviewed changes

bemerybmw requested changes Apr 9, 2026

View reviewed changes

bemerybmw added 12 commits April 9, 2026 13:19

lola: Pass reference to EventDataControlComposite

a3812f3

Since we always create a QM EventDataControl, we pass it into the composite by reference instead of by pointer to avoid doing nullptr checks within the composite (and for clearer semantics).

fixup! lola: Add Service/Event/EventData Control Local structs

308e366

fixup! Remove ControlSlotIndicator and ControlSlotCompositeIndicator

6f98356

lola: Replace magic numbers with variables in tests

49f4239

lola: Change unreachable branch to precondition

3d1b5e3

lola: Fix variable / function naming for clarity

4cfffe3

lola: Assert usable base address is valid instead of cast

eea1858

Since the static_cast can only return a nullptr if the usable base address is a nullptr, we assert that the base address is not a nullptr as this is better reflecting the intention of the assertion.

lola: Remove unused EventDataControl test resources / attorneys

4d70f07

bemerybmw and others added 14 commits April 13, 2026 11:53

lola: Rename function for semantic clarity

af6f193

lola: Update SkeletonMemoryManager

51fb1ec

Updated documentation of functions and general clean up of functions.

lola: Use ScopeExit for method registration guards

c7efe30

We use ScopeExit so that we can rely only the more complicated testing of move construction / move assignment from the ScopeExit class.

lola: Add TransactionLogView

faf7e8b

lola: Add new TransactionLogLocalView tests

2ac3535

mw/com: Clean up sample timestamp usage

e01b6ca

- introduce InvalidTimestamp/FirstValidTimestamp constants - adapt timestamp tests

- removes not used get latest timestamp functionality.

7315849

- moves common attributes to skeleton event common.

muhseth force-pushed the muse_skeleton_get branch from f0c0b32 to a65dc2c Compare April 16, 2026 14:43

muhseth marked this pull request as draft April 16, 2026 14:44

muhseth added 2 commits April 16, 2026 16:58

mw/com/impl: adds GetLatestSlot function

b7e09a4

- Adds GetLatestSlot function to EventDataControlComposite

mw/com/impl: adds GetLatestSample function

636762e

- Adds GetLatestSampleFunction to SkeletonEvent

muhseth force-pushed the muse_skeleton_get branch from a65dc2c to 636762e Compare April 16, 2026 15:24

bemerybmw reviewed Apr 17, 2026

View reviewed changes

Conversation

muhseth commented Mar 18, 2026

Uh oh!

LittleHuba commented Mar 24, 2026

Uh oh!

crimson11 Mar 26, 2026 • edited by LittleHuba Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhseth Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhseth Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bemerybmw Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

crimson11 Mar 26, 2026 •

edited by LittleHuba

Loading

muhseth Apr 2, 2026 •

edited

Loading

muhseth Apr 9, 2026 •

edited

Loading

bemerybmw Apr 17, 2026 •

edited

Loading