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

Eliminate heap allocations in event dispatching #14543

Closed
edrumwri opened this issue Jan 19, 2021 · 5 comments
Closed

Eliminate heap allocations in event dispatching #14543

edrumwri opened this issue Jan 19, 2021 · 5 comments

Comments

@edrumwri
Copy link
Collaborator

edrumwri commented Jan 19, 2021

With some care, heap allocations can generally be avoided (excepting a "warm up" phase) as a Systems' State is updated over time- i.e., through integration, discrete updates, and unrestricted updates- at least with regard to the Systems framework code.

Given the following code:

Simulator sim(my_system);
sim.Initialize()

in which any necessary heap allocations can be performed in Initialize(.), and assuming that my_system performs no heap allocations outside of that warm up period, calls to sim.AdvanceTo(t_final) would ideally allocate no heap.

Heap allocations are presently found in the following places (not comprehensive) encountered by AdvanceTo():

auto merged_events = system_.AllocateCompositeEventCollection();

std::vector<T> times(num_subsystems());

auto event = std::unique_ptr<UnrestrictedUpdateEvent<T>>(this->DoClone());

this->add_event(static_pointer_cast<EventType>(other_event->Clone()));

I'm willing to help eliminate these. Fixing the last two would seem to require a redesign.

@sherm1
Copy link
Member

sherm1 commented Jan 20, 2021

Thanks, @edrumwri !

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 13, 2021
Relevant to: RobotLocomotion#14543, RobotLocomotion#14802

This is the beginning of a PR train to make heapless simulation
possible, with careful system construction.  All the good ideas are
inspired by @edrumwri's PR RobotLocomotion#14707; all the sketchy ones are mine.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 13, 2021
Relevant to: RobotLocomotion#14543, RobotLocomotion#14802

This is the beginning of a PR train to make heapless simulation
possible, with careful system construction.  All the good ideas are
inspired by @edrumwri's PR RobotLocomotion#14707; all the sketchy ones are mine.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 13, 2021
Relevant to: RobotLocomotion#14543, RobotLocomotion#14802

This is the beginning of a PR train to make heapless simulation
possible, with careful system construction.  All the good ideas are
inspired by @edrumwri's PR RobotLocomotion#14707; all the sketchy ones are mine.
soonho-tri pushed a commit that referenced this issue Apr 14, 2021
* simulator: Add test to track heap hygiene

Relevant to: #14543, #14802

This is the beginning of a PR train to make heapless simulation
possible, with careful system construction.  All the good ideas are
inspired by @edrumwri's PR #14707; all the sketchy ones are mine.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 16, 2021
Relevant to: RobotLocomotion#14543

This is the second of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's PR

This patch just moves some method-level heap use into longer-lived
object data; the data flows are the same, but storage gets reused over
successive AdvanceTo() steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 16, 2021
Relevant to: RobotLocomotion#14543

This is the second of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's PR

This patch just moves some method-level heap use into longer-lived
object data; the data flows are the same, but storage gets reused over
successive AdvanceTo() steps.
@rpoyner-tri rpoyner-tri self-assigned this Apr 19, 2021
SeanCurtis-TRI pushed a commit that referenced this issue Apr 20, 2021
Relevant to: #14543

This is the second of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's PR

This patch just moves some method-level heap use into longer-lived
object data; the data flows are the same, but storage gets reused over
successive AdvanceTo() steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 20, 2021
Relevant to: RobotLocomotion#14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch, like the last, just moves some method-level heap use into
longer-lived object data; the data flows are the same, but storage gets
reused over successive simulation steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 22, 2021
Relevant to: RobotLocomotion#14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch, similar to the last, moves some method-level heap use into
longer-lived data managed as a cache entry; the data flows are the same, but
storage gets reused over successive simulation steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 23, 2021
Relevant to: RobotLocomotion#14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch, similar to the last, moves some method-level heap use into
longer-lived data managed as a cache entry; the data flows are the same, but
storage gets reused over successive simulation steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 26, 2021
Relevant to: RobotLocomotion#14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch, similar to the last, moves some method-level heap use into
longer-lived data managed as a cache entry; the data flows are the same, but
storage gets reused over successive simulation steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 26, 2021
Relevant to: RobotLocomotion#14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch, similar to the last, moves some method-level heap use into
longer-lived data managed as a cache entry; the data flows are the same, but
storage gets reused over successive simulation steps.
rpoyner-tri added a commit that referenced this issue Apr 26, 2021
* diagram: Refactor allocation for analyzing events by time

Relevant to: #14543

This is the third of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR #14707.

This patch, similar to the last, moves some method-level heap use into
longer-lived data managed as a cache entry; the data flows are the same, but
storage gets reused over successive simulation steps.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 26, 2021
Relevant to: RobotLocomotion#14543

This is the fourth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch characterizes the heap allocations induced by all forms of
PublishEvents. It also improves naming and commentary.

Subsequent patches will reduce the measured heap usage from simulator
run-time.
@rpoyner-tri
Copy link
Contributor

While fiddling with writing a test for simulator/event heap usage, I discovered a fun (but in retrospect obvious) additional problem: std::vectors will reallocate when they overflow. See #14950 (review) for some discussion from the test case PR.

It might be possible to count or estimate the event population at init time, and do the necessary pre-allocations. At worst (yuck), it might be possible to add API for some event collection size hint that a user could supply. Something along these lines would be necessary to completely eliminate the possibility of heap transactions during AdvanceTo().

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Apr 27, 2021
Relevant to: RobotLocomotion#14543

This is the fourth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch characterizes the heap allocations induced by all forms of
PublishEvents. It also improves naming and commentary.

Subsequent patches will reduce the measured heap usage from simulator
run-time.
@sherm1
Copy link
Member

sherm1 commented Apr 27, 2021

I don't think the goal has to be "no heap allocations in AdvanceTo()". We could make it "no heap allocations in AdvanceTo() for typical uses" (which would include Evan's). For that we could reserve (say) 128 entries in the vectors that hold simultaneous events. I believe it would be a rare simulation that would need more, but all that would happen is a single heap allocation and some copying upon the 129th event, after which nothing would happen until there were 257 simultaneous events!

@rpoyner-tri
Copy link
Contributor

Agreed that we could do a fixed pre-allocation that would cover most cases. I'm content to have that as a fallback, and explore the possibility of system-aware estimates when I get to that step.

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 5, 2021
Relevant to: RobotLocomotion#14543

This is the fifth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch introduces a new callback type for PublishEvents that avoids
heap allocations induced by lambda captures.

Subsequent patches will expand this technique (and test coverage) to the
other event types.

This is a breaking change because it removes the one-argument handle() method
and replaces it with a two-argument form. The known uses inside Drake are
inside the system framework, or in tests. All of those are updated. There are
no known uses outside of Drake.
rpoyner-tri added a commit that referenced this issue May 5, 2021
* framework: Avoid functor allocation for PublishEvents (breaking change)

Relevant to: #14543

This is the fifth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR #14707.

This patch introduces a new callback type for PublishEvents that avoids
heap allocations induced by lambda captures.

Subsequent patches will expand this technique (and test coverage) to the
other event types.

This is a breaking change because it removes the one-argument handle() method
and replaces it with a two-argument form. The known uses inside Drake are
inside the system framework, or in tests. All of those are updated. There are
no known uses outside of Drake.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 5, 2021
Relevant to: RobotLocomotion#14543

This is the sixth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch expands heap-allocation testing to cover all event types and
schedules.

Subsequent patches will remove the heap allocations tracked here.
EricCousineau-TRI pushed a commit that referenced this issue May 10, 2021
Relevant to: #14543

This is the sixth of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR #14707.

This patch expands heap-allocation testing to cover all event types and
schedules.

Subsequent patches will remove the heap allocations tracked here.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 10, 2021
Relevant to: RobotLocomotion#14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch extends the techniques of PR RobotLocomotion#14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 10, 2021
…nge)

Relevant to: RobotLocomotion#14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch extends the techniques of PR RobotLocomotion#14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.

This is a breaking change because it removes the two-argument handle()
methods and replaces them with a three-argument form. The known uses
inside Drake are inside the system framework, or in tests. All of those
are updated. There are no known uses outside of Drake.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 12, 2021
…nge)

Relevant to: RobotLocomotion#14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch extends the techniques of PR RobotLocomotion#14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.

This is a breaking change because it removes the two-argument handle()
methods and replaces them with a three-argument form. The known uses
inside Drake are inside the system framework, or in tests. All of those
are updated. There are no known uses outside of Drake.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 12, 2021
…nge)

Relevant to: RobotLocomotion#14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch extends the techniques of PR RobotLocomotion#14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.

This is a breaking change because it removes the two-argument handle()
methods and replaces them with a three-argument form. The known uses
inside Drake are inside the system framework, or in tests. All of those
are updated. There are no known uses outside of Drake.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 12, 2021
…nge)

Relevant to: RobotLocomotion#14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR RobotLocomotion#14707.

This patch extends the techniques of PR RobotLocomotion#14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.

This is a breaking change because it removes the two-argument handle()
methods and replaces them with a three-argument form. The known uses
inside Drake are inside the system framework, or in tests. All of those
are updated. There are no known uses outside of Drake.
rpoyner-tri added a commit that referenced this issue May 12, 2021
* framework: Avoid functor allocation for all event types (breaking change)

Relevant to: #14543

This is the seventh of a long PR train to make heapless simulation
possible, with careful system construction. Inspired by @edrumwri's
draft PR #14707.

This patch extends the techniques of PR #14969 to all of the event
types.

Subsequent patches will address other sources of heap allocation.

This is a breaking change because it removes the two-argument handle()
methods and replaces them with a three-argument form. The known uses
inside Drake are inside the system framework, or in tests. All of those
are updated. There are no known uses outside of Drake.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 21, 2021
Relevant to: RobotLocomotion#14543

Replace a function-scoped vector with a cache entry. Decline all
invalidation services and use manual methods to ensure no data migrates
between uses.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 21, 2021
Relevant to: RobotLocomotion#14543

Replace a function-scoped vector with a cache entry. Decline all
invalidation services and use manual methods to ensure no data migrates
between uses.
rpoyner-tri added a commit that referenced this issue May 24, 2021
* LeafSystem: remove allocations during simulation

Relevant to: #14543

Replace a function-scoped vector with a cache entry. Decline all
invalidation services and use manual methods to ensure no data migrates
between uses.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 24, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 26, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 26, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 27, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 27, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 27, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 27, 2021
Relevant to: RobotLocomotion#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.
rpoyner-tri added a commit that referenced this issue May 28, 2021
* framework: Remove some allocations from event collections

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.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue May 28, 2021
Relevant to: RobotLocomotion#14543

Remove unnecessary heap allocations from the rest of the event system.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Jun 2, 2021
Relevant to: RobotLocomotion#14543

Remove unnecessary heap allocations from the rest of the event system.
sammy-tri pushed a commit that referenced this issue Jun 2, 2021
Relevant to: #14543

Remove unnecessary heap allocations from the rest of the event system.
@rpoyner-tri
Copy link
Contributor

Looks like all of the heap fixes I had are now merged. Closing.

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

No branches or pull requests

3 participants