dkavolis / Ferram-Aerospace-Research

Aerodynamics model for Kerbal Space Program
Other
77 stars 32 forks source link

reduce inline declarations of variables #13

Closed GER-Space closed 5 years ago

GER-Space commented 5 years ago

When I was reading the code I found several places where vaiables get initialized within often called loops or functions. This will increase the memory garbage created and should be avoided when possible. (it will reduce the stuttering when the GC is run)

good idea not to use a for loop, but then initialized of a placeholder within. nothing is won.

` for (int i = 0; i < _currentAeroModules.Count; i++)

        {

            FARAeroPartModule m = _currentAeroModules[i];
           m.ApplyForces();

        }`

just write:

_currentAeroModules[i].ApplyForces()

or declare m outside of the loop.

dkavolis commented 5 years ago

Thanks for the suggestion, I haven't modified most of the code. You are more than welcome to submit a PR with the changes, it might take a while before I take a look at it myself. I'll add this to my list of future improvements.

GER-Space commented 5 years ago

challenge accepted :-) I had similar problems with Kerbal Konstructs, and messured it there with the memgraph mod. If done correctly it can really improve the framerate.

dkavolis commented 5 years ago

I'd love to improve the performance of FAR, I'm thinking of adding profiler calls in debug builds for some of the methods to (hopefully) find bottlenecks. If you have any other suggestions on how to improve the performance, I'll look into them.

GER-Space commented 5 years ago

I started it already and I'll send you a pr later.

I think that for example the gui values are always calculated, even when the gui is not active.

Its not much, but things are adding up, when it's run every frame.

Other parts of the code I must understand first, before I dare to change them.

GER-Space commented 5 years ago

I can extent some profiling functions to the logging class. I have already some code for it in kerbal konstructs, it's easy to use and I had great results with it. ( Loading time went down from 20s to 0.2)

dkavolis commented 5 years ago

In terms of GUI, I think it should be rewritten using the new Unity GUI system but it's a lot of effort.

siimav commented 5 years ago

Why do you think converting method-scoped variables to class fields makes any difference to GC? Only reference types are garbage-collected and these are usually created with the "new" keyword. All value types are stored on the stack and they have nothing to do with GC. Even moving variable declarations outside of loops or methods do not make any difference.

GER-Space commented 5 years ago

Yes I think so, because the variables are only allocated once. Even when it's going to the stack, it's still an unnecessary allocation, that will be freed afterwards. When they are on class level the same memory segment is used over and over again.

siimav commented 5 years ago

I still can't find any proof on Google that any of that would be beneficial to performance or memory usage. This sounds like something that should be left to to the compiler and/or runtime to handle. And I'm pretty sure that none of these changes make any difference when it comes to garbage collection. To reduce GC pressure you would need to instantiate reference types only once and then update the fields inside them without creating entire new objects. When collections of objects are required, Pooling should be used to reduce heap allocations.

dkavolis commented 5 years ago

I also have not found anything that would suggest moving declarations outside of loops improves performance. Rider/ReSharper even suggests moving declarations inside the loops closer to where the values are assigned. Moving declarations to object scope is ugly and pollutes the completion with unnecessary fields which simply destroys code clarity and maintainability. There is no point in moving declarations far outside the loops they are used in, if anything they should be inside the loops.