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

Conserve 8 bytes per event #11

Closed
pavel-kirienko opened this issue Dec 4, 2024 · 3 comments
Closed

Conserve 8 bytes per event #11

pavel-kirienko opened this issue Dec 4, 2024 · 3 comments
Assignees

Comments

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Dec 4, 2024

Unwrap std::optional<TimePoint>; use TimePoint::min() to represent the inactive event. Roughly like this (untested):

diff --git a/include/olg_scheduler/olg_scheduler.hpp b/include/olg_scheduler/olg_scheduler.hpp
index 52388c6..72dd529 100644
--- a/include/olg_scheduler/olg_scheduler.hpp
+++ b/include/olg_scheduler/olg_scheduler.hpp
@@ -81,14 +81,14 @@ public:
     void cancel() noexcept
     {
         /// It is guaranteed that while an event resides in the tree, it has a valid deadline set.
-        if (deadline_)
+        if (deadline_ != TimePoint::min())
         {
             // Removing a non-existent node from the tree is an UB that may lead to memory corruption,
             // so we have to check first if the event is actually registered.
             remove();
             // This is used to mark the event as unregistered so we don't double-remove it.
             // This can only be done after the event is removed from the tree.
-            deadline_.reset();
+            deadline_ = TimePoint::min();
         }
     }
 
@@ -115,14 +115,14 @@ protected:
     virtual ~Event()
     {
         cancel();
-        assert(!deadline_.has_value());
+        assert(deadline_ == TimePoint::min());
         assert((this->getChildNode(false) == nullptr) && (this->getChildNode(true) == nullptr) &&
                (this->getParentNode() == nullptr));
     }
 
     Event(Event&& other) noexcept :
         cavl::Node<Event>{std::move(static_cast<cavl::Node<Event>&>(other))},
-        deadline_{std::exchange(other.deadline_, std::nullopt)}
+        deadline_{std::exchange(other.deadline_, TimePoint::min())}
     {}
 
     /// Ensure the event is in the tree and set the deadline to the specified absolute time point.
@@ -136,7 +136,7 @@ protected:
             [dead](const Event& other) {
                 /// No two deadlines compare equal, which allows us to have multiple nodes with the same deadline in
                 /// the tree. With two nodes sharing the same deadline, the one added later is considered to be later.
-                return (dead >= other.deadline_.value()) ? +1 : -1;
+                return (dead >= other.deadline_) ? +1 : -1;
             },
             [this] { return this; });
         assert(std::get<0>(ptr_existing) == this);
@@ -151,7 +151,7 @@ protected:
     virtual void execute(const Arg<TimePoint>& args, Tree& tree) = 0;
 
 private:
-    std::optional<TimePoint> deadline_;
+    TimePoint deadline_ = TimePoint::min();
 };

This should be a trivial change because it is entirely self-contained and it shouldn't even affect the tests.

@serges147 fyi

@serges147
Copy link
Collaborator

it's probably only 4 bytes (on 32-bit platform)

and it shouldn't even affect the tests.

probably it won't affect... unless TimePoint::min() was used in tests.

@maksimdrachov
Copy link
Member

This change has allowed us to reduce the callback object size by 8 bytes.

(56 -> 48)

@maksimdrachov
Copy link
Member

Closed by: #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants