CitizenManager extension desync when Manager gets patched by other mods

[krzychu124]

• removed citizen extensions due to issues with de-sync when other mod replace instance of array after caching the ref,
• optimized citizen access by ID by adding ref param where possible and feasible
• replaced const MAX_CITIZEN_COUNT and MAX_INSTANCE_COUNT in loops with a call for current array size (I've heard about some mod(s), still early, WIP, which could lift the game limits, so fixes possible compatibility issues ahead of time)

[chameleon-tbn]

Thank you for taking care of this!

No more isolated static references and therefor no more cache misses
no more possibility of stale references
no more having to rely on static initializer invocation
by removing the extension method we should save some time due to avoid the method call overhead.

lgtm

[krzychu124]

by removing the extension method we should save some time due to avoid the method call overhead.

In CitizenManager.instance the instance is property which is a method under the hood + it has an if condition that runs each call + array buffer is a field of field of the Array32 wrapper instance.
Extension method was using arrayref directly instead of generic<T>->instance(with if)->arrayWrapperField->arrayref IMO It's worse than before but whatever.

[kianzarrin]

@krzychu124 this is a big change. We have similar code for NetNode, NetSegment, Vehicle, ... . if we want to change all of them its going to be uncomfortable!

why not to do this?

private static CitizenUnit[] _citizenUnitBuffer => Singleton<CitizenManager>.instance.m_units.m_buffer;

One downside is when it is used inside a loop then accessing Singleton<CitizenManager>.instance.m_units.m_buffer would have performance penalty. but we could simply avoid using the extensions inside performance critical loops.instead of changing all of TMPE.

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

A third solution is to ask other mods to update Mangers buffers when level is preloaded. then TMPE can simple update the static cache when level starts.

I was aware of this potential issue (of someone expanding the arrays) when I introduced the strategy of caching NetSegment/NetNode buffers but I though if it becomes a problem then we can do one of the above so I went ahead with it anyways.

[krzychu124]

We have similar code for NetNode, NetSegment, Vehicle, ... . if we want to change all of them its going to be uncomfortable!

I don't plan changing any of those since there are unlikely to change and for Vehicles/ParkedVehicles More Vehicles mod replaces the ref when mod is enabled.

why not to do this?

It's the worst possible option because it hides the "expensive" call behind the method and in most cases you can just put array ref in a variable and reuse it.

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

No idea what are you talking about here. Objects are passed by ref. Updating cache every step means doing stuff you don't have to do if you would just use ref when needed.

A third solution is to ask other mods to update Mangers buffers when level is preloaded. then TMPE can simple update the static cache when level starts.

They can't use level preload because they need to work with deserialized data or read it manually so not available at that moment. Also depends on subscription order of the event. If you think about updating before OnLevelLoaded then too late -> mods can try to access data in Serializable extension to check if mod settings are still valid compared to loaded savegame which is called before. The best would be to write prefix for data deserializer and update same as MoreVehicles does (in OnEnabled)

Singleton<>.instance is most expensive call (result from the mono profiler I've managed to setup yesterday)

Time(ms) Count   P/call(ms) Method name
########################
 14938.000  159775    0.093   ColossalFramework.Singleton`1::get_instance()
 ...

[kianzarrin]

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

No idea what are you talking about here. Objects are passed by ref. Updating cache every step means doing stuff you don't have to do if you would just use ref when needed.

I mean like this:

namespace TrafficManager.Util.Extensions
{
    using ColossalFramework;

    public static class CitizenUnitExtensions {
        internal static OnBeforeSimulationStep() =>  _citizenUnitBuffer = Singleton<CitizenManager>.instance.m_units.m_buffer;

        private static CitizenUnit[] _citizenUnitBuffer = Singleton<CitizenManager>.instance.m_units.m_buffer;
        internal static ref CitizenUnit ToCitizenUnit(this uint citizenUnit) => ref _citizenUnitBuffer[citizenUnit];
    }
}

They can't use level preload because they need to work with deserialized data or read it manually so not available at that moment.

Does deserialization release CitizenManager.m_units buffer and then allocate a new one? I was under the impression that it keeps the buffer but modifies its content. and if that is the case, they can simply expand the array before deserialization.

[krzychu124]

I'm aware what you meant and I don't like it/recommend anyone doing this.

they can simply expand the array before deserialization.

They could but they don't. Case-closed.
I'm not going to teach anyone how to write good/better code. That takes a way too much time which could be spend on other things.

[krzychu124]

I need two reviews 😉

[kvakvs]

Is this better? Thought we upgraded most/all direct uses of m_ buffers to helpers?
Also in several other locations, m_ buffers are used

I get it this is to always get the latest ref which may be modified by some mods, but we can update the ref ourselves too.

[krzychu124]

Yes we could update ref ourselves but I see a few problems.

1. When? Between user hit "load" on a savegame to the frame you see in-game screen is a ton of frames where other mods could change ref and ref swap cannot be reliably detected - working on desynced refs will cause weird bugs, assuming we will notice that something is wrong.
2. There is no need to update ref once game loads everything and show the in-game UI.
3. It's hard to benchmark what is the real cost and potentially using cached ref can cause more performance issues than accessing it when needed (in range of method call)

The other managers (Net/Building) are really hard to mod so I doubt anyone try that in near future. Still, there are certain ways how you can do ref swap without breaking anything, although much harder and limits how some things can be handled (e.g. deserialization).

[kianzarrin]

It's the worst possible option because it hides the "expensive" call behind the method and in most cases you can just put array ref in a variable and reuse it.

I could be wrong on this but I don't think all code should be optimal out of the fear it hides performance issues and mistakenly used in future. I think only the critical path should be optimized.

other than that code looks good.