Skip defensive copy in Cell.getState()

[DavidLegg]

No description provided.

[DavidLegg]

I don't think this is an acceptable change. Consider a task that does the following:

var x = read(someCell);
delay(5 minutes);
doSomething(x);

Without the defensive copy here, x is a reference to the same object held by the engine. That object is mutated by step during the delay(5 minutes), such that doSomething observes the wrong value.

[Twisol]

Yes, that's exactly right. The value returned by getState() is meant to be a snapshot of the state at the current time, not a live view. If we could enforce that the returned state objects are not mutable by users, then we could allow a "live view" interpretation safely, and users could make a mutable copy on demand. Since we can't enforce immutability, this would it more complicated to develop correct mission models, as you'd have to both be aware that the state of a cell is a live view and make sure not to mutate it without first taking a copy.

I would expect that most copies returned by getState() are transient and do not graduate from the young generation of the GC, and thus can be collected relatively quickly and with low overhead, so the cost should primarily be that of the time taken to make the copy. Has this been profiled as a bottleneck?

[DavidLegg]

Apologies for the noise on this - I opened this on behalf of @bradNASA, and didn't know how to turn off automatic review requests. Theresa has since told me how to avoid them by manually picking reviewers.
Claude says this is a bottleneck, but I haven't seen the data myself to confirm. Even if it is, I don't think it's the main one we should be targeting.

In case I forget to assign reviewers, if you see a PR targeting perf/clipper-apgen-translation or something similar, feel free to ignore it. That's the dev branch we're using on Clipper's APGen-to-Aerie task.

[Twisol]

Ah, I see! My bad; I saw these PRs because I have notifications on for this repo, not because I was assigned as a reviewer. I only hopped in because this is a part of the codebase where I (think I) can provide value as another set of eyes.

(It could also be useful to open these as "Draft PRs", which I think may also inhibit the automatic assignment of reviewers. I don't know if they prevent notifications, but I find that a PR being marked as "Draft" is usually a good signal that the PR is still in development and isn't necessarily directed at all eyes yet.)

[Twisol]

By the by -- our discussion in #1843 makes me think that there might be an inoffensive version of this change. Both this PR and the stateCache noted in #1843 serve to reduce unnecessary cell copies; but the local cache approach is a much more targeted and tunable method. In particular, we could give each task resumption its own cache, so that every access to a cell within the same resumption acquires the same value. Thus, you only pay the cost of accessing a cell once per task resumption (EDIT: as long as you don't emit to the cell in between, which would invalidate the cache entry), which could be a very big savings if a task occasionally references the same cell many times. (I have in mind, for instance, a cell containing a large HashMap, for which a task does a bunch of small queries against entries in that HashMap; we don't really want to copy the whole map every time!)

This approach would avoid both the "live view" and "sharing" issues discussed earlier. First, the acquired state is not a "live view", because we always access a cell through a copy that never receives new events or steps through time. Second, the only "sharing" that can occur is between a task and itself: if you "mutate" a cell in one place, you can observe that mutation elsewhere within the same resumption, but it will (effectively) get wiped away the next time you delay, and you will never see other tasks' changes. If a child task happens to have a queried variable from its parent in scope (e.g. if you spawn a lambda), then (1) the parent is already expected not to mutate that variable (in fact, whether the mutation is observable depends on whether the parent task is a ThreadedTask or a ReplayingTask), and (2) when the child task reads the same cell, it won't reflect the parent's changes.

I think the minimal self-sharing induced by a resumption-local state cache would not be particularly risky, and there's good potential to significantly reduce the number of copies made over the course of simulation. It might be worth extending the stateCache concept to tasks. The only counterbalances I can see are the cost of logic to invalidate the cache (e.g. if you get(); set(); get(); a cell, then you'll need to make two copies) and any relevant memory pressure (which, if the cache is a good optimization, should be negligible on balance).

(amusingly, this comment does exist in EngineScheduler, so apparently i did think about this way back when:)

// TODO: Cache the return value (until the next emit or until the task yields) to avoid unnecessary copies
//  if the same state is requested multiple times in a row.