From 7b464368bed5b3ca24f9dbd9499dceade54c59d9 Mon Sep 17 00:00:00 2001 From: Karl Ostmo Date: Sun, 10 Mar 2024 16:55:51 -0700 Subject: [PATCH 1/4] add docstrings and refactor for understandability --- .../Testing/1598-detect-entity-change.yaml | 1 + src/swarm-engine/Swarm/Game/Step.hs | 60 ++++++++++++------- 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/data/scenarios/Testing/1598-detect-entity-change.yaml b/data/scenarios/Testing/1598-detect-entity-change.yaml index 604a48b8c..266cfaeb0 100644 --- a/data/scenarios/Testing/1598-detect-entity-change.yaml +++ b/data/scenarios/Testing/1598-detect-entity-change.yaml @@ -40,6 +40,7 @@ robots: - fast grabber - logger - treads + - tweezers inventory: - [1, "dial (R)"] - [1, "dial (G)"] diff --git a/src/swarm-engine/Swarm/Game/Step.hs b/src/swarm-engine/Swarm/Game/Step.hs index ed3c1f462..d295ee93c 100644 --- a/src/swarm-engine/Swarm/Game/Step.hs +++ b/src/swarm-engine/Swarm/Game/Step.hs @@ -140,10 +140,10 @@ finishGameTick = RobotStep SBefore -> temporal . gameStep .= WorldTick RobotStep _ -> void gameTick >> finishGameTick --- | Insert the robot back to robot map. +-- | Update the state of the robot as known by the 'robotMap'. -- Will selfdestruct or put the robot to sleep if it has that set. -insertBackRobot :: Has (State GameState) sig m => RID -> Robot -> m () -insertBackRobot rn rob = do +registerUpdatedRobotState :: Has (State GameState) sig m => RID -> Robot -> m () +registerUpdatedRobotState rn rob = do time <- use $ temporal . ticks zoomRobots $ if rob ^. selfDestruct @@ -164,8 +164,8 @@ type HasGameStepState sig m = (Has (State GameState) sig m, Has (Lift IO) sig m, -- | Run a set of robots - this is used to run robots before/after the focused one. -- -- Note that during the iteration over the supplied robot IDs, it is possible --- that a robot that may have been present in 'robotMap' at the outset --- of the iteration to be removed before the iteration comes upon it. +-- that a robot /that may have been present in 'robotMap' at the outset +-- of the iteration/ to be removed before the iteration comes upon it. -- This is why we must perform a 'robotMap' lookup at each iteration, rather -- than looking up elements from 'robotMap' in bulk up front with something like -- 'restrictKeys'. @@ -182,7 +182,7 @@ runRobotIDs robotNames = do forM_ mr (stepOneRobot rn) where stepOneRobot :: HasGameStepState sig m => RID -> Robot -> m () - stepOneRobot rn rob = tickRobot rob >>= insertBackRobot rn + stepOneRobot rn rob = tickRobot rob >>= registerUpdatedRobotState rn -- | -- Runs the given robots in increasing order of 'RID'. @@ -223,9 +223,18 @@ iterateRobots time f runnableBots = iterateRobots time f $ poolAugmentation remainingBotIDs -- | This is a helper function to do one robot step or run robots before/after. +-- +-- The debugger behavior is determined by which robot is "focused", requiring consideration +-- of certain edge cases: +-- +-- * The player switches focus to a different robot in the middle of a tick +-- * The originally focused robot is destroyed earlier in the tick (before +-- it gets a chance to run) +-- +-- Returns True if the tick is complete; that is, all of the robots +-- have had an opportunity to run. singleStep :: HasGameStepState sig m => SingleStep -> RID -> IS.IntSet -> m Bool -singleStep ss focRID robotSet = do - let (preFoc, focusedActive, postFoc) = IS.splitMember focRID robotSet +singleStep ss focRID robotSet = case ss of ---------------------------------------------------------------------------- -- run robots from the beginning until focused robot @@ -245,24 +254,28 @@ singleStep ss focRID robotSet = do SSingle rid | not focusedActive -> do singleStep (SAfter rid) rid postFoc -- skip inactive focused robot SSingle rid -> do + let focusUnchanged = rid == focRID mOldR <- uses (robotInfo . robotMap) (IM.lookup focRID) case mOldR of - Nothing | rid == focRID -> do + Nothing | focusUnchanged -> do debugLog "The debugged robot does not exist! Exiting single step mode." - runRobotIDs postFoc - temporal . gameStep .= WorldTick - temporal . ticks %= addTicks 1 - return True + finishTickWith WorldTick Nothing | otherwise -> do + -- The original target of debugging is gone, but it's OK because + -- the player changed focus. + -- QUESTION: What if the focus got changed to a robot that + -- has already run in this tick?? debugLog "The previously debugged robot does not exist!" singleStep SBefore focRID postFoc Just oldR -> do -- if focus changed we need to finish the previous robot - newR <- (if rid == focRID then stepRobot else tickRobotRec) oldR - insertBackRobot focRID newR - if rid == focRID + newR <- (if focusUnchanged then stepRobot else tickRobotRec) oldR + registerUpdatedRobotState focRID newR + if focusUnchanged then do when (newR ^. activityCounts . tickStepBudget == 0) $ + -- Our robot ran out of its step budget for this tick, + -- so we will have to continue on to the other robots. temporal . gameStep .= RobotStep (SAfter focRID) return False else do @@ -272,18 +285,23 @@ singleStep ss focRID robotSet = do -- run robots after the focused robot SAfter rid | focRID <= rid -> do -- This state takes care of two possibilities: - -- 1. normal - rid == focRID and we finish the tick + -- 1. normal: rid == focRID and we finish the tick -- 2. changed focus and the newly focused robot has previously run -- so we just finish the tick the same way - runRobotIDs postFoc - temporal . gameStep .= RobotStep SBefore - temporal . ticks %= addTicks 1 - return True + finishTickWith $ RobotStep SBefore SAfter rid | otherwise -> do -- go to single step if new robot is focused let (_pre, postRID) = IS.split rid robotSet singleStep SBefore focRID postRID where + (preFoc, focusedActive, postFoc) = IS.splitMember focRID robotSet + + finishTickWith stepMode = do + runRobotIDs postFoc + temporal . gameStep .= stepMode + temporal . ticks %= addTicks 1 + return True + h = hypotheticalRobot (Out VUnit emptyStore []) 0 debugLog txt = do m <- evalState @Robot h $ createLogEntry RobotError Debug txt From 4d34296d8803fd7ba32ca78f2132edb323f073c1 Mon Sep 17 00:00:00 2001 From: Karl Ostmo Date: Sun, 10 Mar 2024 19:31:37 -0700 Subject: [PATCH 2/4] ensure awakened bots are added to the run pool in debug mode --- src/swarm-engine/Swarm/Game/Step.hs | 47 +++++++++++++++++------------ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/swarm-engine/Swarm/Game/Step.hs b/src/swarm-engine/Swarm/Game/Step.hs index d295ee93c..8b28116ae 100644 --- a/src/swarm-engine/Swarm/Game/Step.hs +++ b/src/swarm-engine/Swarm/Game/Step.hs @@ -184,6 +184,26 @@ runRobotIDs robotNames = do stepOneRobot :: HasGameStepState sig m => RID -> Robot -> m () stepOneRobot rn rob = tickRobot rob >>= registerUpdatedRobotState rn +-- | This function must be utilized in every loop that iterates over +-- robots and invokes 'stepRobot' (i.e. steps a robot's CESK machine). +getExtraRunnableRobots :: Has (State GameState) sig m => TickNumber -> m (IS.IntSet -> IS.IntSet) +getExtraRunnableRobots time = do + -- NOTE: We could use 'IS.split thisRobotId activeRIDsThisTick' + -- to ensure that we only insert RIDs greater than 'thisRobotId' + -- into the queue. + -- However, we already ensure in 'wakeWatchingRobots' that only + -- robots with a larger RID are scheduled for the current tick; + -- robots with smaller RIDs will be scheduled for the next tick. + robotsToAdd <- use $ robotInfo . currentTickWakeableBots + if null robotsToAdd + then return id + else do + -- We have awakened new robots in the current robot's iteration, + -- so obtain a function to add them to the remaining iteration pool. + zoomRobots $ wakeUpRobotsDoneSleeping time + robotInfo . currentTickWakeableBots .= [] + return $ IS.union $ IS.fromList robotsToAdd + -- | -- Runs the given robots in increasing order of 'RID'. -- @@ -202,25 +222,8 @@ iterateRobots :: HasGameStepState sig m => TickNumber -> (RID -> m ()) -> IS.Int iterateRobots time f runnableBots = forM_ (IS.minView runnableBots) $ \(thisRobotId, remainingBotIDs) -> do f thisRobotId - - -- We may have awakened new robots in the current robot's iteration, - -- so we add them to the list - poolAugmentation <- do - -- NOTE: We could use 'IS.split thisRobotId activeRIDsThisTick' - -- to ensure that we only insert RIDs greater than 'thisRobotId' - -- into the queue. - -- However, we already ensure in 'wakeWatchingRobots' that only - -- robots with a larger RID are scheduled for the current tick; - -- robots with smaller RIDs will be scheduled for the next tick. - robotsToAdd <- use $ robotInfo . currentTickWakeableBots - if null robotsToAdd - then return id - else do - zoomRobots $ wakeUpRobotsDoneSleeping time - robotInfo . currentTickWakeableBots .= [] - return $ IS.union $ IS.fromList robotsToAdd - - iterateRobots time f $ poolAugmentation remainingBotIDs + augmentRunPool <- getExtraRunnableRobots time + iterateRobots time f $ augmentRunPool remainingBotIDs -- | This is a helper function to do one robot step or run robots before/after. -- @@ -271,6 +274,10 @@ singleStep ss focRID robotSet = -- if focus changed we need to finish the previous robot newR <- (if focusUnchanged then stepRobot else tickRobotRec) oldR registerUpdatedRobotState focRID newR + + time <- use $ temporal . ticks + augmentRunPool <- getExtraRunnableRobots time + if focusUnchanged then do when (newR ^. activityCounts . tickStepBudget == 0) $ @@ -280,7 +287,7 @@ singleStep ss focRID robotSet = return False else do -- continue to newly focused - singleStep SBefore focRID postFoc + singleStep SBefore focRID $ augmentRunPool postFoc ---------------------------------------------------------------------------- -- run robots after the focused robot SAfter rid | focRID <= rid -> do From 25118635f303b9fa878d5b1fb3161f161df5d577 Mon Sep 17 00:00:00 2001 From: Karl Ostmo Date: Sun, 10 Mar 2024 19:12:22 -0700 Subject: [PATCH 3/4] [EXPERIMENTAL] RID parameter to halt iteration in iterateRobots --- src/swarm-engine/Swarm/Game/Step.hs | 48 +++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/src/swarm-engine/Swarm/Game/Step.hs b/src/swarm-engine/Swarm/Game/Step.hs index 8b28116ae..08cef363f 100644 --- a/src/swarm-engine/Swarm/Game/Step.hs +++ b/src/swarm-engine/Swarm/Game/Step.hs @@ -94,7 +94,7 @@ gameTick = do ticked <- use (temporal . gameStep) >>= \case WorldTick -> do - runRobotIDs active + void $ runRobotIDs Nothing active temporal . ticks %= addTicks 1 pure True RobotStep ss -> singleStep ss focusedRob active @@ -174,10 +174,15 @@ type HasGameStepState sig m = (Has (State GameState) sig m, Has (Lift IO) sig m, -- -- * Every tick, every active robot shall have exactly one opportunity to run. -- * The sequence in which robots are chosen to run is by increasing order of 'RID'. -runRobotIDs :: HasGameStepState sig m => IS.IntSet -> m () -runRobotIDs robotNames = do +runRobotIDs :: + HasGameStepState sig m => + -- Optional RID to stop before + Maybe RID -> + IS.IntSet -> + m IS.IntSet +runRobotIDs maybeStopBeforeRID robotNames = do time <- use $ temporal . ticks - flip (iterateRobots time) robotNames $ \rn -> do + flip (iterateRobots maybeStopBeforeRID time) robotNames $ \rn -> do mr <- uses (robotInfo . robotMap) (IM.lookup rn) forM_ mr (stepOneRobot rn) where @@ -217,13 +222,29 @@ getExtraRunnableRobots time = do -- /splitting/ the min item from rest of the queue is still an O(log N) operation, -- and therefore is not any better than the 'minView' function from 'IntSet'. -- --- Tail-recursive. -iterateRobots :: HasGameStepState sig m => TickNumber -> (RID -> m ()) -> IS.IntSet -> m () -iterateRobots time f runnableBots = - forM_ (IS.minView runnableBots) $ \(thisRobotId, remainingBotIDs) -> do +-- To support the interactive debugger, we also support the capability to +-- stop before reaching a certain 'RID', in which case we return the +-- remaining 'RID's we didn't get to yet. +iterateRobots :: + HasGameStepState sig m => + -- Optional RID to stop before + Maybe RID -> + TickNumber -> + (RID -> m ()) -> + IS.IntSet -> + m IS.IntSet +iterateRobots maybeStopBeforeRID time f runnableBots = + case IS.minView runnableBots of + Nothing -> return runnableBots + Just (thisRobotId, remainingBotIDs) -> + if maybe True (< thisRobotId) maybeStopBeforeRID + then doRecurse thisRobotId remainingBotIDs + else return runnableBots + where + doRecurse thisRobotId remainingBotIDs = do f thisRobotId augmentRunPool <- getExtraRunnableRobots time - iterateRobots time f $ augmentRunPool remainingBotIDs + iterateRobots maybeStopBeforeRID time f $ augmentRunPool remainingBotIDs -- | This is a helper function to do one robot step or run robots before/after. -- @@ -242,7 +263,8 @@ singleStep ss focRID robotSet = ---------------------------------------------------------------------------- -- run robots from the beginning until focused robot SBefore -> do - runRobotIDs preFoc + remainingBotsForAfterward <- runRobotIDs (Just focRID) preFoc + temporal . gameStep .= RobotStep (SSingle focRID) -- also set ticks of focused robot steps <- use $ temporal . robotStepsPerTick @@ -250,7 +272,7 @@ singleStep ss focRID robotSet = -- continue to focused robot if there were no previous robots -- DO NOT SKIP THE ROBOT SETUP above if IS.null preFoc - then singleStep (SSingle focRID) focRID robotSet + then singleStep (SSingle focRID) focRID $ IS.union remainingBotsForAfterward robotSet else return False ---------------------------------------------------------------------------- -- run single step of the focused robot (may skip if inactive) @@ -275,6 +297,8 @@ singleStep ss focRID robotSet = newR <- (if focusUnchanged then stepRobot else tickRobotRec) oldR registerUpdatedRobotState focRID newR + -- This may have introduced more robots to run! + -- Need to check 'currentTickWakeableBots' and empty it. time <- use $ temporal . ticks augmentRunPool <- getExtraRunnableRobots time @@ -304,7 +328,7 @@ singleStep ss focRID robotSet = (preFoc, focusedActive, postFoc) = IS.splitMember focRID robotSet finishTickWith stepMode = do - runRobotIDs postFoc + void $ runRobotIDs Nothing postFoc temporal . gameStep .= stepMode temporal . ticks %= addTicks 1 return True From 219727ed92d89ac8db39209df48547cb688bc81f Mon Sep 17 00:00:00 2001 From: Karl Ostmo Date: Mon, 11 Mar 2024 23:18:45 -0700 Subject: [PATCH 4/4] add performance note --- src/swarm-engine/Swarm/Game/State/Robot.hs | 7 ++++- src/swarm-engine/Swarm/Game/Step.hs | 31 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/swarm-engine/Swarm/Game/State/Robot.hs b/src/swarm-engine/Swarm/Game/State/Robot.hs index d066b3d8e..c3290abfa 100644 --- a/src/swarm-engine/Swarm/Game/State/Robot.hs +++ b/src/swarm-engine/Swarm/Game/State/Robot.hs @@ -392,7 +392,12 @@ wakeWatchingRobots myID currentTick loc = do newInsertions = M.filter (not . null) $ M.fromList wakeTimeGroups -- Contract: This must be emptied immediately - -- in 'iterateRobots' + -- in 'iterateRobots'. + -- + -- Tracking the wakeups due on the current tick separately from other + -- waiting robots is a performance optimization (see #1736); + -- it avoids an O(log N) 'Map' lookup on 'internalWaitingRobots' in favor + -- of an O(1) 'null' check in the common case. currentTickWakeableBots .= currTickWakeable -- NOTE: There are two "sources of truth" for the waiting state of robots: diff --git a/src/swarm-engine/Swarm/Game/Step.hs b/src/swarm-engine/Swarm/Game/Step.hs index 08cef363f..a7c697341 100644 --- a/src/swarm-engine/Swarm/Game/Step.hs +++ b/src/swarm-engine/Swarm/Game/Step.hs @@ -191,6 +191,37 @@ runRobotIDs maybeStopBeforeRID robotNames = do -- | This function must be utilized in every loop that iterates over -- robots and invokes 'stepRobot' (i.e. steps a robot's CESK machine). +-- +-- = Performance notes +-- +-- == Guarding the 'Map' lookup in 'wakeUpRobotsDoneSleeping' +-- +-- We use the extra 'currentTickWakeableBots' collection +-- to supplement the 'internalWaitingRobots' Map +-- so that we can avoid a 'Map' lookup in the common case. +-- +-- See comment in the body of 'wakeWatchingRobots'. +-- +-- == Avoiding a linear 'union' operation? +-- +-- Note that although this: +-- @ +-- IS.union somePopulatedSet usuallyEmptySet +-- @ +-- +-- produces the same output as this: +-- @ +-- if null usuallyEmptySet +-- then somePopulatedSet +-- else IS.union somePopulatedSet usuallyEmptySet +-- @ +-- +-- It may be the case that the shorter code would end up doing an O(N) operation every time, +-- rather than only when the "usuallyEmptySet" is empty. +-- Therefore, aside from the primary motivation of 'currentTickWakeableBots' described above, +-- it is also useful for us to "guard" the 'union' operation with an O(1) 'null' check. +-- +-- TODO: Actually verify this claim with benchmarks? getExtraRunnableRobots :: Has (State GameState) sig m => TickNumber -> m (IS.IntSet -> IS.IntSet) getExtraRunnableRobots time = do -- NOTE: We could use 'IS.split thisRobotId activeRIDsThisTick'