OpenPerpetuum / PerpetuumServer

The Open Perpetuum Project's fork of the Perpetuum Standalone Server
https://openperpetuum.com
Other
44 stars 21 forks source link

Combine Weather/Gametime effects #244

Closed MikeJeffers closed 3 years ago

MikeJeffers commented 3 years ago

Same end result, with one GUI icon, one effect active at a time, same logic w.r.t time and weather. Requires: https://github.com/OpenPerpetuum/OPDB/pull/238

nref-dan commented 3 years ago

I think the EnvironEffectHandler is a bit over-optimized. I'm pretty sure that the current DI structure will load up all zones, which will create all of the event handlers. So even though this deferred loading is designed to save resources it would actually be less efficient with all the deferred calls than just new'ing up a dictionary of event processors for each zone.

I think this type of event handler is best to just have loaded for all zones at all times anyway, since it could become an integral part of the zones, and the zones are designed to be "always on" because there's robot activity, etc.

Although you're not incorrect in general cases, this usage of Lazy Init is designed to speed up startup time. You're deferring the initialising cost until well after startup. Even though all zones are loaded, the Lazy initialiser won't run until the weather effect is actually needed and grabbed from the dict. In the end, all the effects will be loaded in memory and theoretically never released. The benefit here is that you're not adding even more time to the Zone startup (which is already long enough)

DerekJarvis commented 3 years ago

My response is similar, in theory I think you're right: it should speed up the load time and defer it until after - but in my tests of trying to pick apart the server and load just one thing, it's pretty much impossible. It all gets loaded, and I'm pretty sure it spends more CPU cycles resolving the lazy loading than it would cost to just load it directly. Are there any performance benchmarks on this? This is hardly conclusive, but this post (http://stackoverflow.com/a/21491131/340045) shows that when the work being done is trivial, the Eager Init + First Fetch is less time than Lazy Init + First fetch due to the overhead being added by Lazy. So if we know for certain that it's going to immediately fetch it (it is), then it's not going to speed it up, it's going to slow it down.

nref-dan commented 3 years ago

My response is similar, in theory I think you're right: it should speed up the load time and defer it until after - but in my tests of trying to pick apart the server and load just one thing, it's pretty much impossible. It all gets loaded, and I'm pretty sure it spends more CPU cycles resolving the lazy loading than it would cost to just load it directly. Are there any performance benchmarks on this? This is hardly conclusive, but this post (http://stackoverflow.com/a/21491131/340045) shows that when the work being done is trivial, the Eager Init + First Fetch is less time than Lazy Init + First fetch due to the overhead being added by Lazy. So if we know for certain that it's going to immediately fetch it (it is), then it's not going to speed it up, it's going to slow it down.

That's kind of missing the point. Eager Init First Fetch is definitely faster, because first fetch on a lazy actually performs the init. The point here is not to increase performance of the system, but to defer the initialisation until you really need it because we don't want to keep stacking up program startup time.

Also it's not going to immediately fetch all the weather effects. If it is, then this implementation is broken and we should fix that.

Essentially, the Lazy Init is only run when GetEffect retrieves that value from the dictionary. So if you never call GetEffect for a particular weather effect it should not be initialised.