Use a mutable struct for the state of GridSpaceIdIterator (#936)

Author: Tortar

CHANGELOG.md

@@ -10,18 +10,22 @@ _We tried to deprecate every major change, resulting in practically no breakage
   - Allows us to develop new types of models that may have rules that are defined differently, without being based on e.g., two particular functions.
   - Allows us to develop (in the future) a new model type that is optimized for multi-agent simulations.
 - The `@agent` macro has been rewritten to support fields with default and const values. It has a new usage syntax now that parallelizes more Julia's native `struct` declaration. The old macro version still works but it's deprecated. Since now the macro supports these features, using `@agent` is the only supported way to create agent types for Agents.jl.
-- Manually setting or altering the ids of agents is no longer allowed. The agent id is now considered a read-only field, and is set internally by Agents.jl to enable hidden optimizations in the future. As a consequence, `add_agent!(agent::AbstractAgent, pos::ValidPos, model::ABM)`  and `add_agent!(agent::AbstractAgent, model::ABM)` have been deprecated. See issue #861 for more.
-  - Due to this, the `nextid` function is no longer public API.
+- Manually setting or altering the ids of agents is no longer allowed. The agent id is now considered a read-only field, and is set internally by Agents.jl to enable hidden optimizations in the future. As a consequence, `add_agent!(agent::AbstractAgent, pos::ValidPos, model::ABM)`  and `add_agent!(agent::AbstractAgent, model::ABM)` have been deprecated. See issue #861 for more. Due to this, the `nextid` function is no longer public API.
 - Agent types in `ContinuousSpace` now use `SVector` for their `pos` and `vel` fields rather than `NTuple`. `NTuple` usage in `ContinuousSpace` is officially deprecated, but backward compatibility is *mostly* maintained. Known breakages include the comparison of agent position and/or velocity with user-defined tuples, e.g., doing `agent.pos == (0.5, 0.5)`. This will always be `false` in v6 as `agent.pos` is an `SVector`. The rest of the functionality should all work without problems, such as moving agents to tuple-based positions etc.
 
 ## New features
 
+- Grid and continuous spaces support boundaries with mixed periodicity, specified by tuples with a `Bool` value for each dimension, e.g. `GridSpace((5,5); periodic=(true,false))` is periodic along the first dimension but not along the second.
 - Two new functions `random_id_in_position` and `random_agent_in_position` can be used to select a random id/agent in a position in discrete spaces (even with filtering).
 - A new function `swap_agents` can be used to swap an agents couple in a discrete space.
+
+## Performance Improvements
+
 - A new argument `alloc` can be used to select a more performant version in relation to the expensiveness of the filtering for all random methods selecting ids/agents/positions.
 - The `random_agent` function is now much faster than before. The functions `random_nearby_position`, `random_nearby_id` and `random_nearby_agent` are up to 2 times faster thanks to a faster sampling function.
+- The `nearby_agents` function for `ContinuousSpace` and `GridSpace` is now 1.5x faster than before.
 - The `sample!` function is up to 2x faster than before.
-- Grid and continuous spaces support boundaries with mixed periodicity, specified by tuples with a `Bool` value for each dimension, e.g. `GridSpace((5,5); periodic=(true,false))` is periodic along the first dimension but not along the second.
+
 
 # v5.17
 

src/spaces/grid_multi.jl

@@ -120,8 +120,11 @@ function nearby_ids(pos::NTuple{D, Int}, space::GridSpace{D,P}, r::Real = 1) whe
     return GridSpaceIdIterator{P, D}(stored_ids, nindices, pos, L, space_size, nocheck)
 end
 
-# Iterator struct. State is `(pos_i, inner_i)` with `pos_i` the index to the nearby indices
-# P is Boolean, and means "periodic".
+mutable struct IdIteratorState
+    pos_i::Int
+    inner_i::Int
+    ids_in_pos::Vector{Int}
+end
 struct GridSpaceIdIterator{P,D}
     stored_ids::Array{Vector{Int},D}  # Reference to array in grid space
     indices::Vector{NTuple{D,Int}}    # Result of `offsets_within_radius` pretty much
@@ -136,10 +139,9 @@ Base.IteratorSize(::Type{<:GridSpaceIdIterator}) = Base.SizeUnknown()
 # Initialize iteration
 function Base.iterate(iter::GridSpaceIdIterator)
     @inbounds begin
-    stored_ids, indices, L, origin, nocheck = getproperty.(
-        Ref(iter), (:stored_ids, :indices, :L, :origin, :nocheck))
-    combine, invalid = nocheck ? (combine_positions_nocheck, invalid_access_nocheck) :
-                                 (combine_positions, invalid_access)
+    indices, L, origin = iter.indices, iter.L, iter.origin
+    combine, invalid = iter.nocheck ? (combine_positions_nocheck, invalid_access_nocheck) :
+                                      (combine_positions, invalid_access)
     pos_i = 1
     pos_index = combine(indices[pos_i], origin, iter)
     # First, check if the position index is valid (bounds checking)
@@ -151,22 +153,21 @@ function Base.iterate(iter::GridSpaceIdIterator)
         pos_index = combine(indices[pos_i], origin, iter)
     end
     # We have a valid position index and a non-empty position
-    ids_in_pos = stored_ids[pos_index...]
+    ids_in_pos = iter.stored_ids[pos_index...]
     id = ids_in_pos[1]
     end
-    return (id, (pos_i, 2, ids_in_pos))
+    return (id, IdIteratorState(pos_i, 2, ids_in_pos))
 end
 
 # For performance we need a different method of starting the iteration
 # and another one that continues iteration. Second case uses the explicitly
 # known knowledge of `pos_i` being a valid position index.
 function Base.iterate(iter::GridSpaceIdIterator, state)
     @inbounds begin
-    stored_ids, indices, L, origin, nocheck = getproperty.(
-        Ref(iter), (:stored_ids, :indices, :L, :origin, :nocheck))
-    combine, invalid = nocheck ? (combine_positions_nocheck, invalid_access_nocheck) :
-                                 (combine_positions, invalid_access)
-    pos_i, inner_i, ids_in_pos = state
+    indices, L, origin = iter.indices, iter.L, iter.origin
+    combine, invalid = iter.nocheck ? (combine_positions_nocheck, invalid_access_nocheck) :
+                                      (combine_positions, invalid_access)
+    pos_i, inner_i, ids_in_pos = state.pos_i, state.inner_i, state.ids_in_pos
     if inner_i > length(ids_in_pos)
         # we have exhausted IDs in current position, so we reset and go to next
         pos_i += 1
@@ -180,11 +181,12 @@ function Base.iterate(iter::GridSpaceIdIterator, state)
             pos_i > L && return nothing
             pos_index = combine(indices[pos_i], origin, iter)
         end
-        ids_in_pos = stored_ids[pos_index...]
+        ids_in_pos = iter.stored_ids[pos_index...]
     end
     # We reached the next valid position and non-empty position
     id = ids_in_pos[inner_i]
-    return (id, (pos_i, inner_i + 1, ids_in_pos))
+    state.pos_i, state.inner_i, state.ids_in_pos = pos_i, inner_i + 1, ids_in_pos
+    return (id, state)
     end
 end