Tmpe: Eliminate all static variables and methods
_As a developer I want my application to be hot-swappable to allow for a faster development time._
Assumptions/Preconditions
- Static variables and methods prevent the mod from being modified and updated while the game is running: C:S cannot fully unload old instances.
- Other features relevant for hot-swappability, like loading/saving custom data or deployment/removal of method detours are triggered at specific OnLoad/OnUnload events.
Tasks
- Remove as many static fields/methods as possible. Convert them into instance-level members.
- If not everything can be converted, collect remaining members and put them at a central place.
- Extend OnLoad/OnUnload event handlers such they take care that any remaining static members are unset/set.
- Check if the mod can be hot-swapped. If any further problems arise that were not foreseen create an issue on GitHub.
Acceptance criteria
- Static members are gone, except for a limited amount
- Remaining static members are instantiated/invalidated at mod loading/unloading time
- Mod functionality remains unchanged
- Mod loading/unloading still works
VictorPhilipp
All 17 comments
As a novice learning C#, may I ask why? (it will be useful knowledge of a mod I'm writing).
aubergine10
on 4 Mar 2019
Also, could we get a linter (or whatever) set up to help us with this task?
aubergine10
on 5 Mar 2019
I don't see the purpose in this. There's always a need every once in a while for a static variable and/or method. Just look in Colossal Order's code; They use them often for things that don't require an instance.
FireController1847
on 6 Mar 2019
Ah, I think I get it - if user has 2 copies of TMPE installed (eg. Labs and Stable, or one of those and local build, or one of those and pcfantasy mods that replicate chunks of TMPE), it's all using the same namespaces so the static stuff from one is overwriting that of the other?
aubergine10
on 6 Mar 2019
@aubergine10 Exactly, issue #210 proofs that C:S is unable to handle multiple mod instances correctly regarding static variables/methods. The second instance of TMPE (the one from the AJR mod) tries to access a static variable (Options.turnOnRed) that is present in AJR's TMPE but not in the other instance.
By removing all static members (except for a very limited & controlled set, in case we need it) we also can make the mod 100% hot-swappable. Then, we would not have to restart the game on every code change.
VictorPhilipp
on 6 Mar 2019
Also, could we get a linter (or whatever) set up to help us with this task?
Could you add an issue for this? Maybe also add a linter / rule scheme that we can use.
VictorPhilipp
on 6 Mar 2019
I have never had to restart the game on a code change, I've only had to exit and re-enter the world. If you want I could start work on this while you guys work on Harmony integration, and then once that's done I can work on EVE
FireController1847
on 6 Mar 2019
Remaining static members are instantiated/invalidated at mod loading/unloading time
I assume instantiation occurs automatically due to classes / members being defined as static?
How would such things be invalidated when unloading?
aubergine10
on 7 Mar 2019
@aubergine10 Probably setting them as null in the classes, giving them a value at load, and setting them back to null and unload
FireController1847
on 7 Mar 2019
Another reason: I would like to use a mocking framework (probably Moq since it's free) for unit testing. However, it does not support mocking static methods.
VictorPhilipp
on 17 Mar 2019
Note that 'true' hot swapping is impossible.
The game uses a single app domain both for the game itself and for all mods. Unloading an assembly from an app domain is not supported on Mono (it will be supported only on .NET Core 3.0 and .NET 5).
Even if there will be no static references anymore, the types of the 'old' assembly will still be in the app domain and thus can be instantiated. Having 10 'hot swaps` will result in 10 copies of an assembly loaded into the same app domain (provided that every new assembly copy gets a new version number; otherwise, the game's logic won't load such an assembly).
But even that won't help much, because the game uses assembly resolution based on assembly names. See the CurrentDomain_AssemblyResolve method in the BuildConfig class. So if some code uses Reflection to obtain an instance of a type, the game might provide a wrong type from a 'zombie' assembly that is not a part of the 'hot-swapped' mod instance, thus causing weird exceptions like "Cannot cast object of type MyType to MyType" and so on.
dymanoid
on 1 Jul 2019
What is wrong with static methods?
They are just functions without global/static state.
Static method calls are 10 times faster than ~instance~ virtual method calls.
OK with instance calls though.
Some (most of them really) static methods have no instance to bind to by design.
kvakvs
on 19 Jul 2019
@kvakvs Not to challenge your thoughts, but I'd love to see some references for the claims that they're 10 times faster.
FireController1847
on 19 Jul 2019
@FireController1847 We were discussing terrible GameObject.SendMessage performance with @krzychu124 and here's what he referred to https://jacksondunstan.com/articles/3605
Ok virtual calls are super expensive, instance calls are not too much, just an extra argument passed.
kvakvs
on 19 Jul 2019
With the next couple of updates to mod checker (#434 and #439) issues with zombie TM:PE assemblies will be significantly reduced. That being the case, and factoring in that hot-swappability can't reliably be achieved (as per Dymanoid's comment above), should we be rethinking our stance on static methods, classes, etc?
aubergine10
on 19 Jul 2019
There is nothing wrong with static methods, especially when they are pure functions (read the definition of that if you are not sure about this term). However, they don't allow polymorphism.
If you are running into performance troubles because of virtual method calls only, please let me see such an application. In 99.999% of the cases the reason is somewhere else. The measurements in the referenced blog post are crap, they measure everything but the method call performance. Just tell me if you want more info.
The static fields, properties, and methods were suspected in the original post to prevent the hot swapping, but the real reason has nothing to do with static members and methods. I explained it in my previous comment above.
However, static fields and properties are evil from design point of view. They should be avoided. They make the code less testable, they reduce the code maintainability and break many software design principles, like e. g. SOLID.
dymanoid
on 19 Jul 2019
As of v11-alpha3, the mod checker will always report duplicate instances of TM:PE even if the mod checker has been disabled by user.
aubergine10
on 1 Aug 2019
Related issues
aubergine10
路
32Comments
FireController1847
路
69Comments
kianzarrin
路
38Comments
aubergine10
路
29Comments
aubergine10
路
39Comments
Most helpful comment
There is nothing wrong with static methods, especially when they are pure functions (read the definition of that if you are not sure about this term). However, they don't allow polymorphism.
If you are running into performance troubles because of virtual method calls only, please let me see such an application. In 99.999% of the cases the reason is somewhere else. The measurements in the referenced blog post are crap, they measure everything but the method call performance. Just tell me if you want more info.
The static fields, properties, and methods were suspected in the original post to prevent the hot swapping, but the real reason has nothing to do with static members and methods. I explained it in my previous comment above.
However, static fields and properties are evil from design point of view. They should be avoided. They make the code less testable, they reduce the code maintainability and break many software design principles, like e. g. SOLID.