Skip to content

[BUG]: Fix double-iteration of deferred LINQ in Map.cs GetRandom methods #399

@efrosim

Description

@efrosim

Before You Report

  • I have searched for existing reports of this bug, and this bug report is unique.

Version

1.1.6

Description

In LabApi\Features\Wrappers\Facility\Map.cs, multiple methods responsible for getting random objects (e.g., GetRandomRoom, GetRandomDoor, GetRandomLight, GetRandomCamera, GetRandomRagdoll) suffer from a severe double-iteration performance issue and unnecessary GC allocations due to the misuse of LINQ.

  1. Double Iteration O(N): When filtering collections using .Where(), the resulting IEnumerable<T> is deferred. Calling .Count() forces the enumerator to iterate through the entire collection. Immediately after, calling .ElementAt(index) forces a second iteration from the beginning up to the random index.
  2. GC Allocations: Using .Where(x => x.Zone == zone) allocates a closure (to capture the zone variable) and an enumerator on the heap every time the method is called.

For large collections (like Doors or RoomLights), this doubles the CPU overhead and generates garbage, which degrades server performance and causes GC spikes if called frequently by plugins.

To Reproduce

Review the source code in LabApi\Features\Wrappers\Facility\Map.cs. For example, in GetRandomRoom(FacilityZone zone):

public static Room? GetRandomRoom(FacilityZone zone)
{
    // 1. Allocates closure and enumerator
    IEnumerable<Room> rooms = Rooms.Where(x => x.Zone == zone);
    
    // 2. First iteration: goes through ALL rooms to count them
    int count = rooms.Count(); 
    
    // 3. Second iteration: goes through the rooms AGAIN up to the random index
    return count != 0 ? rooms.ElementAt(UnityEngine.Random.Range(0, count)) : null; 
}

This exact pattern is copy-pasted across at least 10 different methods in the Map class.

Expected Behavior

Fetching a random element should be a true zero-allocation operation. The collection should be iterated exactly once (O(N)), without generating any LINQ-related garbage, and pooled lists should be safely returned using a try...finally block.

Additional Information

Proposed Fix:

To achieve true zero-allocation and optimal performance, replace the LINQ query with a standard foreach loop, populate a rented pooled list, pick a random element, and safely return the list to the pool inside a finally block.

Change this pattern:

public static Room? GetRandomRoom(FacilityZone zone)
{
    IEnumerable<Room> rooms = Rooms.Where(x => x.Zone == zone);
    int count = rooms.Count();
    return count != 0 ? rooms.ElementAt(UnityEngine.Random.Range(0, count)) : null;
}

To this:

using NorthwoodLib.Pools;

public static Room? GetRandomRoom(FacilityZone zone)
{
    List<Room> validRooms = ListPool<Room>.Shared.Rent();
    
    try
    {
        foreach (Room room in Rooms)
        {
            if (room.Zone == zone)
            {
                validRooms.Add(room);
            }
        }
        
        return validRooms.Count > 0 
            ? validRooms[UnityEngine.Random.Range(0, validRooms.Count)] 
            : null;
    }
    finally
    {
        ListPool<Room>.Shared.Return(validRooms);
    }
}

(This fix should be applied to all GetRandom... methods in Map.cs that currently use .Count() followed by .ElementAt() on a deferred IEnumerable).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions