Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
react immediately to wakeups (#1736)
Fixes #1598 ## Demo Illustrates immediate (same-tick) reaction to a change of a `watch`ed cell: scripts/play.sh -i data/scenarios/Testing/1598-detect-entity-change.yaml --autoplay --speed 1 ## Background Robots can be **scheduled** to wakeup from a `watch` by the `wakeWatchingRobots` function. Robots may be **actually awakened** by the `wakeUpRobotsDoneSleeping` function. Previously, `wakeWatchingRobots` would only ever schedule wakeups at `currentTick + 1`. Then, once the next tick is reached, in `wakeUpRobotsDoneSleeping` only robots that had been scheduled to wake on precisely the **current tick** could actually be awakened. But this made it impossible for one robot to cause another robot, which had been sleeping, to actually wake up within the same tick that scheduling of the wakeup is performed. The importance of this comes into play for system robots that are `watch`ing a cell and intended to react instantaneously to player actions (usually by running code in an `instant` block). During each "tick", every active robot gets exactly one "turn" to execute code. Given the following ID assignments: | Robot ID | Robot name | | --- | --- | | `0` | `base` | | `1` | `systemBot` | the `systemBot` will always execute *after* the `base` robot in a given tick. If the `systemBot` is `watch`ing a given cell, a modification of that cell by the `base` will schedule a wakeup of the `systemBot`. If the scheduled wakeup tick is `currentTick + 1`, then the `base` will have **another execution turn** before the `systemBot` gets an opportunity to react to the `base`'s action at the current tick, causing the `systemBot` to overlook actions that may be relevant to a goal condition. ## Solution In contast, if we schedule the `systemBot` wakeup for `currentTick` instead of `currentTick + 1`, then it should have an opportunity to wake, run, and react to the `base`'s action before the `base`'s next turn. But in the status quo, the selection of which robots to run in a given tick is made only once, before iteration over those robots begins. We need instead to update the robots-to-run pool dynamically, after each iteration on an individual robot. The most appropriate data structure for the iteration pool is probably a [Monotone priority queue](https://en.wikipedia.org/wiki/Monotone_priority_queue), but we approximate this with an `IntSet` of `RID`s and the [`minView`](https://hackage.haskell.org/package/containers-0.7/docs/Data-IntSet.html#v:minView) function. Being able to alter the list of active robots in the middle of a given tick's robot iteration has required a change to the `runRobotIDs` function. Instead of using a `forM_` to iterate over a static list of `RIDs`, we have a custom iteration function `iterateRobots` to handle the dynamic set. ## Performance Benchmarks were performed locally with `stack` and GHC 9.6. I had tested the replacement of the `forM_` loop with `iterateRobots` in isolation, and it had zero effect on benchmarks. But some other trivial changes tested in isolation, such as the addition of docstrings, yielded a +6% increase in benchmarks. So there seems to be some instability in the ability of the compiler to perform certain optimizations. With this PR, increases in benchmark times ranging from 7% to 11% were observed. The obvious culprit is that `wakeUpRobotsDoneSleeping` is now called N times per tick, rather than once per tick, where N is the number of robots that are initially (or become) active during that tick. ### Mitigating with an "awakened" set In the common case (i.e. when there are *no* watching robots), the `wakeUpRobotsDoneSleeping` that is now executed `N` times per tick (instead of once per tick) incurs a `Map` lookup, which is `O(log M)` in the number of distinct future wakeup times across all robots. However, those extra `N` invocations only exist to serve the purpose of `watch`ing robots that may need to wake up at the current tick. It may be more efficient to have a dedicated `Set` in the `GameState` for such robots that gets populated parallel to this insertion: https://github.com/swarm-game/swarm/blob/ad5c58917e058306d33e86f3b85e7825d64e54f4/src/swarm-engine/Swarm/Game/State/Robot.hs#L387 Then if this set remains `null`, we can avoid paying for an additional `O(N * log M)` operations entailed by the `Map` lookups into `internalWaitingRobots`. #### Result Indeed, storing awakened bots in a new `currentTickWakeableBots` set restored the benchmarks nearly to the baseline. Instead of the regressions in the 10-20% range observed before, now only a few benchmarks had at most 3-4% increases. ### CI regressions In CI, (`snake`, `powerset`, and `word-search`) exceeded their timeout thresholds in GHC 9.2 and GHC 9.4. No regressions were observed with GHC 9.6. To accommodate the lowest common denominator, I bumped the thresholds for those three scenarios to get a passing build. I don't believe it is worth further effort or investigation to optimize for GHC 9.4, since such efforts will be moot for GHC 9.6 and onward. Test invocation: cabal test swarm-integration --test-show-details streaming --test-options '--color always --pattern "word-search"' | **Scenario** | GHC 9.4.8 | GHC 9.4.8 | GHC 9.6.4 | GHC 9.6.4 | | --- | --- | --- | --- | --- | | | Before | **After** | Before | **After** | `snake` | 1.84s | **5.38s** | 1.62s | **1.67s** | | `powerset` | 1.66s | **5.09s** | 1.56s | **1.66s** | | `word-search` | 0.56s | **1.91s** | 0.44s | **0.48s** | ### Potential improvements #### `TickNumber` as `Map` key `waitingRobots` is of type `Map TickNumber [RID]`. Instead of `Map`, we could have a `newtype` that encapsulates a `IntMap` to make lookups by `TickNumber` more efficient.
- Loading branch information
