CitiesSkylinesMods / TMPE

Cities: Skylines Traffic Manager: President Edition
https://steamcommunity.com/sharedfiles/filedetails/?id=1637663252
MIT License
574 stars 85 forks source link

Cache computed values in `Options.cs` #1515

Open originalfoo opened 2 years ago

originalfoo commented 2 years ago

There's a couple of methods in Options.cs that are likely being called from hotpath - I assume return values will get cached by JIT or something, but it wouldn't harm to just cache the values when option values change...

_Originally posted by @aubergine10 in https://github.com/CitiesSkylinesMods/TMPE/pull/1512#discussion_r842025062_

getRecklessDriverModulo()

This converts reckless driver drop-down value in to a modulo. We should calculate it whenever reckless dirver option changes and cache result in an int field.

Additionally, code intent can be clarified with switch expression:

internal static int RecklessDriverModulo(RecklessDrivers level) => level switch
{
    RecklessDrivers.PathOfEvil => 10,
    RecklessDrivers.RushHour => 20,
    RecklessDrivers.MinorComplaints => 50,
    RecklessDrivers.HolyCity => 10000,
    _ => 10000,
};

IsDynamicLaneSelectionActive()

This can be invoked whenever the associated options (Advanced Vehicle AI checkbox and DLS slider) change and result cached in a bool field.

krzychu124 commented 2 years ago

That's the compiled code from your example. I have no idea what do you want to achieve 🤷‍♂️

image with enum image

originalfoo commented 2 years ago

The switch expression is just less boilerplaete for humans reading the code; nothing to do with performace.

The main thing is caching the return values as it looks to me like call sites are hotpath (or at least very frequently called), for example:

Obtaining cached value from field will be faster than function call?

krzychu124 commented 2 years ago

Obtaining cached value from field will be faster than function call?

Not always, but... why not enum with values (xyz = 2, abc = 10 etc)? Value will be compiled as int so zero impact on performance, and you can also compare them like this Enum.Value1 < Enum.Value2 if necessary (see SimulationAccuracy).

image

IL code image

originalfoo commented 2 years ago

Would love to do that, but it would affect de/serialization which is currently limited to byte values :(

krzychu124 commented 2 years ago

We are going to change serialization so there is a chance (since you've set the issue as low priority) 😉

originalfoo commented 2 years ago

Another enum value that would be useful is for overlays - I've been chipping away at a centralised OverlayManager which will enable TM:PE overlays to be used by other mods and non-TM:PE tools such as road tool, bulldozer, etc., thus providing situational awareness to user when they are altering networks. It relies on an enum to control what persistent/active overlays are visible at any time:

        public enum Overlays {
            None = 0,

            PrioritySigns = 1 << 0,
            TrafficLights = 1 << 1,
            SpeedLimits = 1 << 2,
            VehicleRestrictions = 1 << 3,
            ParkingRestrictions = 1 << 4,
            ParkingSpaces = 1 << 5, // future
            JunctionRestrictions = 1 << 6,
            LaneConnectors = 1 << 7,

            // When active show underground ovlerays in overground mode
            Tunnels = 1 << 8,

            Common =
                PrioritySigns | TrafficLights | SpeedLimits |
                VehicleRestrictions | ParkingRestrictions |
                JunctionRestrictions | LaneConnectors,

            Bulk =
                PrioritySigns | JunctionRestrictions |
                SpeedLimits | LaneConnectors,

            // Developer/niche overlays
            Nodes = 1 << 25,
            Lanes = 1 << 26,
            Vehicles = 1 << 27,
            PathUnits = 1 << 28, // future
            Citizens = 1 << 29,
            Buildings = 1 << 30,

            // TM:PE use only - special flag that denotes user choices in Overlays tab
            TMPE = 1 << 31,
        }