Reuse EngineQuerier to reduce allocation pressure#1843
Conversation
| 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(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
No description provided.