Skip to content

Commit

Permalink
framework: Remove some allocations from event collections
Browse files Browse the repository at this point in the history
Relevant to: #14543

Rewrite the storage of event collections to avoid most allocations,
while maintaining most of the pre-existing public API. To support this
change, allow fully-derived-type events to be copied and assigned. The
Clone() mechanism is still supported, primarily for interfacing with
Python.

This patch also deprecates EventCollection<E>::add_event() and any
overrides.
  • Loading branch information
rpoyner-tri committed May 26, 2021
1 parent 5dbcaf6 commit 817f848
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 63 deletions.
2 changes: 1 addition & 1 deletion manipulation/kuka_iiwa/iiwa_command_receiver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void IiwaCommandReceiver::DoCalcNextUpdateTime(
// Schedule a discrete update event at now to latch the current position.
*time = context.get_time();
auto& discrete_events = events->get_mutable_discrete_update_events();
discrete_events.add_event(std::make_unique<DiscreteUpdateEvent<double>>(
discrete_events.AddEvent(DiscreteUpdateEvent<double>(
[this](const Context<double>& event_context,
const DiscreteUpdateEvent<double>&,
DiscreteValues<double>* next_values) {
Expand Down
2 changes: 1 addition & 1 deletion systems/analysis/test/simulator_limit_malloc_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ GTEST_TEST(SimulatorLimitMallocTest,
// heap-free simulation after initialization, given careful system
// construction.
// TODO(rpoyner-tri): whittle allocations down to 0.
test::LimitMalloc heap_alloc_checker({.max_num_allocations = 63});
test::LimitMalloc heap_alloc_checker({.max_num_allocations = 21});
simulator.AdvanceTo(1.0);
simulator.AdvanceTo(2.0);
simulator.AdvanceTo(3.0);
Expand Down
34 changes: 6 additions & 28 deletions systems/framework/event.h
Original file line number Diff line number Diff line change
Expand Up @@ -443,13 +443,6 @@ class Event {
virtual ~Event() {}
#endif

/** @name Does not allow move or assignment; copy constructor is private. */
///@{
void operator=(const Event&) = delete;
Event(Event&&) = delete;
void operator=(Event&&) = delete;
///@}

// TODO(eric.cousineau): Deprecate and remove this alias.
using TriggerType = systems::TriggerType;

Expand Down Expand Up @@ -485,7 +478,7 @@ class Event {
* can be nullptr, which means this event does not have any associated
* data.
*/
EventData* get_mutable_event_data() { return event_data_.get(); }
EventData* get_mutable_event_data() { return event_data_.get_mutable(); }

// Note: Users should not be calling this.
#if !defined(DRAKE_DOXYGEN_CXX)
Expand Down Expand Up @@ -527,10 +520,7 @@ class Event {
}

protected:
Event(const Event& other) : trigger_type_(other.trigger_type_) {
if (other.event_data_ != nullptr)
set_event_data(other.event_data_->Clone());
}
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Event);

// Note: Users should not be calling this.
#if !defined(DRAKE_DOXYGEN_CXX)
Expand All @@ -554,7 +544,7 @@ class Event {

private:
TriggerType trigger_type_;
std::unique_ptr<EventData> event_data_{nullptr};
copyable_unique_ptr<EventData> event_data_{nullptr};
};

/**
Expand Down Expand Up @@ -607,9 +597,7 @@ struct PeriodicEventDataComparator {
template <typename T>
class PublishEvent final : public Event<T> {
public:
void operator=(const PublishEvent&) = delete;
PublishEvent(PublishEvent&&) = delete;
void operator=(PublishEvent&&) = delete;
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(PublishEvent);
bool is_discrete_update() const override { return false; }

/** @name Publish Callbacks
Expand Down Expand Up @@ -682,8 +670,6 @@ class PublishEvent final : public Event<T> {
}

private:
PublishEvent(const PublishEvent&) = default;

void DoAddToComposite(TriggerType trigger_type,
CompositeEventCollection<T>* events) const final {
auto event = std::unique_ptr<PublishEvent<T>>(this->DoClone());
Expand Down Expand Up @@ -713,9 +699,7 @@ class PublishEvent final : public Event<T> {
template <typename T>
class DiscreteUpdateEvent final : public Event<T> {
public:
void operator=(const DiscreteUpdateEvent&) = delete;
DiscreteUpdateEvent(DiscreteUpdateEvent&&) = delete;
void operator=(DiscreteUpdateEvent&&) = delete;
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(DiscreteUpdateEvent);
bool is_discrete_update() const override { return true; }

/** @name Discrete Update Callbacks
Expand Down Expand Up @@ -794,8 +778,6 @@ class DiscreteUpdateEvent final : public Event<T> {
}

private:
DiscreteUpdateEvent(const DiscreteUpdateEvent&) = default;

void DoAddToComposite(TriggerType trigger_type,
CompositeEventCollection<T>* events) const final {
auto event = std::unique_ptr<DiscreteUpdateEvent<T>>(this->DoClone());
Expand Down Expand Up @@ -825,9 +807,7 @@ class DiscreteUpdateEvent final : public Event<T> {
template <typename T>
class UnrestrictedUpdateEvent final : public Event<T> {
public:
void operator=(const UnrestrictedUpdateEvent&) = delete;
UnrestrictedUpdateEvent(UnrestrictedUpdateEvent&&) = delete;
void operator=(UnrestrictedUpdateEvent&&) = delete;
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(UnrestrictedUpdateEvent);
bool is_discrete_update() const override { return false; }

/** @name Unrestricted Update Callbacks
Expand Down Expand Up @@ -906,8 +886,6 @@ class UnrestrictedUpdateEvent final : public Event<T> {
}

private:
UnrestrictedUpdateEvent(const UnrestrictedUpdateEvent&) = default;

void DoAddToComposite(TriggerType trigger_type,
CompositeEventCollection<T>* events) const final {
auto event = std::unique_ptr<UnrestrictedUpdateEvent<T>>(this->DoClone());
Expand Down
91 changes: 68 additions & 23 deletions systems/framework/event_collection.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,16 @@ class EventCollection {
* does not permit adding new events. Derived classes must implement this
* method to add the specified event to the homogeneous event collection.
*/
DRAKE_DEPRECATED("2021-09-01", "Use AddEvent instead.")
virtual void add_event(std::unique_ptr<EventType> event) = 0;

/**
* Adds an event to this collection, or throws if the concrete collection
* does not permit adding new events. Derived classes must implement this
* method to add the specified event to the homogeneous event collection.
*/
virtual void AddEvent(EventType event) = 0;

protected:
/**
* Constructor only accessible by derived class.
Expand Down Expand Up @@ -182,10 +190,17 @@ class DiagramEventCollection final : public EventCollection<EventType> {
/**
* Throws if called, because no events should be added at the Diagram level.
*/
void add_event(std::unique_ptr<EventType>) override {
void add_event(std::unique_ptr<EventType>) final {
throw std::logic_error("DiagramEventCollection::add_event is not allowed");
}

/**
* Throws if called, because no events should be added at the Diagram level.
*/
void AddEvent(EventType) final {
throw std::logic_error("DiagramEventCollection::AddEvent is not allowed");
}

/**
* Returns the number of constituent EventCollection objects that correspond
* to each subsystem in the Diagram.
Expand Down Expand Up @@ -245,7 +260,7 @@ class DiagramEventCollection final : public EventCollection<EventType> {
/**
* Clears all subevent collections.
*/
void Clear() override {
void Clear() final {
for (EventCollection<EventType>* subevent : subevent_collection_) {
subevent->Clear();
}
Expand All @@ -255,7 +270,7 @@ class DiagramEventCollection final : public EventCollection<EventType> {
* Returns `true` if and only if any of the subevent collections have any
* events.
*/
bool HasEvents() const override {
bool HasEvents() const final {
for (const EventCollection<EventType>* subevent : subevent_collection_) {
if (subevent->HasEvents()) return true;
}
Expand All @@ -274,7 +289,7 @@ class DiagramEventCollection final : public EventCollection<EventType> {
* DiagramEventCollection.
*/
void DoAddToEnd(
const EventCollection<EventType>& other_collection) override {
const EventCollection<EventType>& other_collection) final {
const DiagramEventCollection<EventType>& other =
dynamic_cast<const DiagramEventCollection<EventType>&>(
other_collection);
Expand All @@ -301,12 +316,21 @@ class DiagramEventCollection final : public EventCollection<EventType> {
template <typename EventType>
class LeafEventCollection final : public EventCollection<EventType> {
public:
/**
* The default capacity of event storage allocation, expressed as a number of
* events. Chosen to be large enough that most systems won't need to allocate
* during simulation advance steps.
*/
static constexpr int kDefaultCapacity = 32;

DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(LeafEventCollection)

/**
* Constructor.
*/
LeafEventCollection() = default;
LeafEventCollection() {
Reserve(kDefaultCapacity);
}

/**
* Static method that generates a LeafEventCollection with exactly
Expand All @@ -316,44 +340,65 @@ class LeafEventCollection final : public EventCollection<EventType> {
static std::unique_ptr<LeafEventCollection<EventType>>
MakeForcedEventCollection() {
auto ret = std::make_unique<LeafEventCollection<EventType>>();
auto event = std::make_unique<EventType>(EventType::TriggerType::kForced);
ret->add_event(std::move(event));
EventType event(EventType::TriggerType::kForced);
ret->AddEvent(event);
return ret;
}

/**
* Reserve storage for at least `capacity` events. At construction, there
* will be at least `kDefaultCapacity`; use this method to reserve more.
*/
void Reserve(int capacity) {
events_storage_.reserve(capacity);
events_.reserve(capacity);
}

/**
* Returns a const reference to the vector of const pointers to all of the
* events.
*/
// TODO(siyuan): provide an iterator instead.
const std::vector<const EventType*>& get_events() const { return events_; }
const std::vector<const EventType*>& get_events() const {
events_.clear();
for (const auto& event : events_storage_) {
events_.push_back(&event);
}
DRAKE_ASSERT(events_.empty() || events_[0] == &events_storage_[0]);
return events_;
}

/**
* Add `event` to the existing collection. Ownership of `event` is
* transferred. Aborts if event is null.
*/
void add_event(std::unique_ptr<EventType> event) override {
void add_event(std::unique_ptr<EventType> event) final {
DRAKE_DEMAND(event != nullptr);
owned_events_.push_back(std::move(event));
events_.push_back(owned_events_.back().get());
AddEvent(*event);
}

/**
* Add `event` to the existing collection.
*/
void AddEvent(EventType event) final {
events_storage_.push_back(std::move(event));
}

/**
* Returns `true` if and only if this collection is nonempty.
*/
bool HasEvents() const override { return !events_.empty(); }
bool HasEvents() const final { return !events_storage_.empty(); }

/**
* Removes all events from this collection.
*/
void Clear() override {
owned_events_.clear();
events_.clear();
void Clear() final {
events_storage_.clear();
}

protected:
/**
* All events in `other_collection` are concatanated to this.
* All events in `other_collection` are concatenated to this.
*
* Here is an example. Suppose this collection stores the following events:
* <pre>
Expand All @@ -371,23 +416,23 @@ class LeafEventCollection final : public EventCollection<EventType> {
* @throws std::bad_cast if `other_collection` is not an instance of
* LeafEventCollection.
*/
void DoAddToEnd(const EventCollection<EventType>& other_collection) override {
void DoAddToEnd(const EventCollection<EventType>& other_collection) final {
const LeafEventCollection<EventType>& other =
dynamic_cast<const LeafEventCollection<EventType>&>(other_collection);

const std::vector<const EventType*>& other_events = other.get_events();
for (const EventType* other_event : other_events) {
this->add_event(static_pointer_cast<EventType>(other_event->Clone()));
this->AddEvent(*other_event);
}
}

private:
// Owned event unique pointers.
std::vector<std::unique_ptr<EventType>> owned_events_;
std::vector<EventType> events_storage_;

// Points to the corresponding unique pointers. This is primarily used for
// get_events().
std::vector<const EventType*> events_;
// Scratch space for pointers to the corresponding storage entries. This is
// primarily used for get_events(), but persists to avoid re-allocating the
// memory.
mutable std::vector<const EventType*> events_;
};

/**
Expand Down
12 changes: 6 additions & 6 deletions systems/framework/leaf_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -943,15 +943,15 @@ class LeafSystem : public System<T> {
DRAKE_DEMAND(publish != nullptr);

// Instantiate the event.
auto forced = std::make_unique<PublishEvent<T>>(
PublishEvent<T> forced(
TriggerType::kForced,
[this_ptr, publish](const Context<T>& context, const PublishEvent<T>&) {
// TODO(sherm1) Forward the return status.
(this_ptr->*publish)(context); // Ignore return status for now.
});

// Add the event to the collection of forced publish events.
this->get_mutable_forced_publish_events().add_event(std::move(forced));
this->get_mutable_forced_publish_events().AddEvent(std::move(forced));
}

/** Declares a function that is called whenever a user directly calls
Expand Down Expand Up @@ -981,7 +981,7 @@ class LeafSystem : public System<T> {
DRAKE_DEMAND(update != nullptr);

// Instantiate the event.
auto forced = std::make_unique<DiscreteUpdateEvent<T>>(
DiscreteUpdateEvent<T> forced(
TriggerType::kForced,
[this_ptr, update](const Context<T>& context,
const DiscreteUpdateEvent<T>&,
Expand All @@ -992,7 +992,7 @@ class LeafSystem : public System<T> {
});

// Add the event to the collection of forced discrete update events.
this->get_mutable_forced_discrete_update_events().add_event(
this->get_mutable_forced_discrete_update_events().AddEvent(
std::move(forced));
}

Expand Down Expand Up @@ -1023,7 +1023,7 @@ class LeafSystem : public System<T> {
DRAKE_DEMAND(update != nullptr);

// Instantiate the event.
auto forced = std::make_unique<UnrestrictedUpdateEvent<T>>(
UnrestrictedUpdateEvent<T> forced(
TriggerType::kForced,
[this_ptr, update](const Context<T>& context,
const UnrestrictedUpdateEvent<T>&, State<T>* state) {
Expand All @@ -1032,7 +1032,7 @@ class LeafSystem : public System<T> {
});

// Add the event to the collection of forced unrestricted update events.
this->get_mutable_forced_unrestricted_update_events().add_event(
this->get_mutable_forced_unrestricted_update_events().AddEvent(
std::move(forced));
}
//@}
Expand Down
4 changes: 2 additions & 2 deletions systems/lcm/lcm_log_playback_system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ void LcmLogPlaybackSystem::DoCalcNextUpdateTime(

// Schedule a publish event at the next message time.
*time = next_message_time;
events->get_mutable_publish_events().add_event(
std::make_unique<systems::PublishEvent<double>>(
events->get_mutable_publish_events().AddEvent(
systems::PublishEvent<double>(
TriggerType::kTimed, callback));
}

Expand Down
4 changes: 2 additions & 2 deletions systems/lcm/lcm_subscriber_system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ void LcmSubscriberSystem::DoCalcNextUpdateTime(
*time = context.get_time();
EventCollection<UnrestrictedUpdateEvent<double>>& uu_events =
events->get_mutable_unrestricted_update_events();
uu_events.add_event(
std::make_unique<systems::UnrestrictedUpdateEvent<double>>(
uu_events.AddEvent(
systems::UnrestrictedUpdateEvent<double>(
TriggerType::kTimed, callback));
}

Expand Down

0 comments on commit 817f848

Please sign in to comment.