Skip to content

Reuse EngineQuerier to reduce allocation pressure #1843

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.

Sign up for GitHub

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

Open
DavidLegg wants to merge 1 commit into
base: perf/clipper-apgen-translation
Choose a base branch
from
Open

Reuse EngineQuerier to reduce allocation pressure #1843

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions merlin-driver/src/main/java/gov/nasa/jpl/aerie/merlin/driver/engine/SimulationEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ public final class SimulationEngine implements AutoCloseable {
private final LiveCells cells;
private Duration elapsedTime;

private final EngineQuerier reusableQuerier = new EngineQuerier();

public SimulationEngine(LiveCells initialCells) {
timeline = new TemporalEventSource();
referenceTimeline = new TemporalEventSource();
Expand Down Expand Up @@ -523,15 +525,15 @@ public void updateCondition(
final Duration horizonTime
) {
if (this.closed) throw new IllegalStateException("Cannot update condition on closed simulation engine");
final var querier = new EngineQuerier(frame);
reusableQuerier.reset(frame);
final var prediction = this.conditions
.get(condition)
.nextSatisfied(querier, horizonTime.minus(currentTime))
.nextSatisfied(reusableQuerier, horizonTime.minus(currentTime))
.map(currentTime::plus);

this.waitingConditions.subscribeQuery(condition, querier.referencedTopics);
this.waitingConditions.subscribeQuery(condition, reusableQuerier.referencedTopics);

final var expiry = querier.expiry.map(currentTime::plus);
final var expiry = reusableQuerier.expiry.map(currentTime::plus);
if (prediction.isPresent() && (expiry.isEmpty() || prediction.get().shorterThan(expiry.get()))) {
this.scheduledJobs.schedule(JobId.forSignal(condition), SubInstant.Tasks.at(prediction.get()));
} else {
Expand All @@ -548,16 +550,16 @@ public void updateResource(
final Duration currentTime,
final ResourceUpdates resourceUpdates) {
if (this.closed) throw new IllegalStateException("Cannot update resource on closed simulation engine");
final var querier = new EngineQuerier(frame);
reusableQuerier.reset(frame);
resourceUpdates.add(new ResourceUpdates.ResourceUpdate<>(
querier,
reusableQuerier,
currentTime,
resourceId,
this.resources.get(resourceId)));

this.waitingResources.subscribeQuery(resourceId, querier.referencedTopics);
this.waitingResources.subscribeQuery(resourceId, reusableQuerier.referencedTopics);

final var expiry = querier.expiry.map(currentTime::plus);
final var expiry = reusableQuerier.expiry.map(currentTime::plus);
if (expiry.isPresent()) {
this.scheduledJobs.schedule(JobId.forResource(resourceId), SubInstant.Resources.at(expiry.get()));
}
Expand Down Expand Up @@ -1160,15 +1162,19 @@ private static <EventType> Optional<SerializedValue> trySerializeEvent(

/** A handle for processing requests from a modeled resource or condition. */
private static final class EngineQuerier implements Querier {
private final TaskFrame<JobId> frame;
private final Set<Topic<?>> referencedTopics = new HashSet<>();
private TaskFrame<JobId> frame;
private Set<Topic<?>> referencedTopics = new HashSet<>();
private Optional<Duration> expiry = Optional.empty();
// Cache: query -> state. Safe because resources/conditions never emit events during getDynamics,
// so cell state cannot change between repeated reads within one evaluation.
private final HashMap<Query<?>, Object> stateCache = new HashMap<>(32);

public EngineQuerier(final TaskFrame<JobId> frame) {
void reset(final TaskFrame<JobId> frame) {
this.frame = Objects.requireNonNull(frame);
// subscribeQuery takes ownership of the previous set, so allocate a new one
this.referencedTopics = new HashSet<>();
this.expiry = Optional.empty();
this.stateCache.clear();
}
Comment on lines +1172 to 1178

@DavidLegg DavidLegg Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does almost as much work as building a new engine querier, with the added complexity that now, because this is stateful, there's a chance we forget to reset the querier when needed. I'd like to see profiling data showing that this change makes a noticeable improvement before accepting this.

@Twisol Twisol Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'll also note that, like on the other PR, a querier shouldn't be a very long-lived object, since it only exists for the duration of a condition or resource invocation. The point of a generational GC is to deal with short-lived objects more efficiently, so without profiling I'd be skeptical that, out of all of the transient objects created over the course of a simulation, the querier is a particular hotspot.)

@DavidLegg DavidLegg Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking to Brad a little more, it seems that Claude thinks the main benefit here is in keeping stateCache around. That initial size of 32, according to Claude, puts non-negligible pressure on the GC to allocate and deallocate it so often.

In my own profiling, I've seen updateCondition come up as a hot path before, so the idea that allocations in that function might drive performance isn't totally crazy to me. I'll work with Brad to see if we can get more detailed data on this.

@Twisol Twisol Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. It looks like the stateCache is new to perf/clipper-apgen-translation (via 12db906), correct? Do we know whether that improved performance at the time? How was the initial size chosen -- does the cache tend to get filled or does it often need to get resized even larger? The idea of a query cache is a good one in principle, but it seems like we're optimizing an optimization, so it might be worth revisiting the original decision holistically.

The other thing I'd say is, by design, this cache probably saves a lot of unnecessary state copies (and, unlike in the other PR, resources and conditions should never be maintaining local mutable state, so there's little to no danger to resources sharing the same cached copy). We're basically saving an allocation for every cache hit, so the cache's own allocation should matter less exactly if the cache is a good optimization.

On the other hand, constantly flushing and recreating the cache during the same simulation instant works directionally against the optimization niche the cache targets. In principle, this cache could be lifted all the way to the engine toplevel, and maintained over the course of a whole SimulationInstant (covering both conditions and resources). The cache could then be passed to the constructor of EngineQuerier instead of maintaining and resetting a whole mutable EngineQuerier.

Might be worth investigating. There's a lot of hypotheticals floating around this however, so (and I think we agree here) profiling data is really essential for good decision-making. At minimum, I think pulling the cache out to the engine toplevel and passing it to EngineQuerier on construction would be a reasonable way to scope the GC and maintenance impact of the cache. The choice of cache invalidation policy would be orthogonal.


@Override
Expand Down
Loading