Beta 4, final updates?

[arekbulski]

I will write a complete review after sleep, and probably commit something more, but here are basic updates that I just made in notepad. Still to do gameplay check.

[linuxgurugamer]

Why do you make changes which are essentially meaningless? I specically refer to replacing the simple string concatation I use with the String.Format() calls. They are identical, except for speed, and in this game, even though it's not a lot, all mods need to optimize their speed as much as possible. For longer strings, i agree that the String.Format() is sometimes more readable, but if there is no formatting to be done, it's slower.

The code where the String.Format is actually doing something useful, like: String.Format("{0:N1} is fine. It's the new changes where all it's doing is doing concatenation which is not.

I've written a class to test this, it shows that the "+" operator uses about 2/3 of the time that the Format statement uses. This is consistant when testing 1 through 10 strings.

I've added the StringTest.cs class to the project and pushed it into Github, so you can download it and test for yourself. It's disabled with an #if false/#endif because it takes about 30 seconds to run, and I have it set to run upon entering the space scene.

The test also shows that using the StringBuilder is actually a bit faster than concatenation, but not by a lot.

This is also why I try not to use LINQ. Squad did a huge rewrite a while ago, replacing all the "foreach" statements with the "for (int i = 0; i < xxx.Count; i++)" statements because of the performance boost, both from the fact it's a bit faster AND reduced memory wastage.

You are removing comments which are there to provide whitespace because they are important. Excessive whitespace is useless, but whitespace is useful, and when specifically added by comments, it should be left

Also, while I don't see any errors in this code, I would rather accept patches which have been tested. I fixed several things in your previous patch which simple playtesting would have shown.

Finally, I had put spaces between the numbers and the units (ie: iN, kM, s) for readability. The "s" can stay, but please put back the space for the iN, kM

[linuxgurugamer]

Here are the results of the timing test. I have a very fast system, on slower systems, it will be more pronounced. The test was also looking at memory, but that is miniscule, so I'm ignoring that.

Timing results: Operator +, #: 1, Memory: 410861568/437940224 - 410869760 Time: 30 Timing results: Operator +, #: 2, Memory: 410869760/469356544 - 410877952 Time: 61 Timing results: Operator +, #: 3, Memory: 410869760/500789248 - 410877952 Time: 92 Timing results: Operator +, #: 4, Memory: 410873856/533258240 - 410894336 Time: 122 Timing results: Operator +, #: 5, Memory: 410886144/566308864 - 410923008 Time: 152 Timing results: Operator +, #: 6, Memory: 410910720/599363584 - 410927104 Time: 185 Timing results: Operator +, #: 7, Memory: 410918912/426598400 - 410927104 Time: 254 Timing results: Operator +, #: 8, Memory: 410918912/459636736 - 410927104 Time: 280 Timing results: Operator +, #: 9, Memory: 410918912/492687360 - 410931200 Time: 324 Timing results: Format, #: 1, Memory: 410927104/445370368 - 410935296 Time: 45 Timing results: Format, #: 2, Memory: 410931200/486854656 - 410939392 Time: 98 Timing results: Format, #: 3, Memory: 410935296/529031168 - 410939392 Time: 150 Timing results: Format, #: 4, Memory: 410931200/571211776 - 410939392 Time: 213 Timing results: Format, #: 5, Memory: 410931200/410955776 - 410931200 Time: 303 Timing results: Format, #: 6, Memory: 410927104/448024576 - 410939392 Time: 345 Timing results: Format, #: 7, Memory: 410931200/490205184 - 410939392 Time: 439 Timing results: Format, #: 8, Memory: 410931200/532381696 - 410939392 Time: 456 Timing results: Format, #: 9, Memory: 410931200/574558208 - 410935296 Time: 512 Timing results: StringBuilder, #: 1, Memory: 410927104/440147968 - 410939392 Time: 27 Timing results: StringBuilder, #: 2, Memory: 410931200/474304512 - 410939392 Time: 56 Timing results: StringBuilder, #: 3, Memory: 410931200/508461056 - 410939392 Time: 86 Timing results: StringBuilder, #: 4, Memory: 410927104/542613504 - 410935296 Time: 121 Timing results: StringBuilder, #: 5, Memory: 410927104/576765952 - 410935296 Time: 154 Timing results: StringBuilder, #: 6, Memory: 410927104/610922496 - 410935296 Time: 180 Timing results: StringBuilder, #: 7, Memory: 410927104/436752384 - 410935296 Time: 247 Timing results: StringBuilder, #: 8, Memory: 410927104/470908928 - 410935296 Time: 279 Timing results: StringBuilder, #: 9, Memory: 410927104/505061376 - 410931200 Time: 307

[arekbulski]

Let me explain by background and intentions, maybe that will shed some light.

I am sort of an evangelist of Python (previously C#) and there are many things that I know "in theory". One of those is consistency. If we have String.Format most of the code, I would like to have it in all. If we used no spece in "0.0iN", I would like to have same in all texts. I would appreciate if you paid a little more appreciation to that. Just please. :) I am fine with space added, will commit it tomorrow.

Second is foreach, its much more readable than for loop. Readability usually influences maintainability and reliability of code, so its not meaningless. Performance wise, Microsoft clearly stated that those loops are identical in speed. In theory. In practice well, since Unity runs on some godaweful ancient version of Mono (not .NET) that is so crap that it should have not existed in the first place, thats is something I would not bother to compare empirically. I had an interesting experience on the KSP forum, where I pointed out that .NET generational GC can easily handle temporary instances, making guys "improvement" meaningless, he countered with the fact that Unity runs on Mono, oooold mark and sweep GC. I think you get the point.

Third is enum eqality. Both equality and inequality run use short evaluation. It is better in readability to state the correct states that the opposite. Also enum eq is as cheap as integer eq. 2~4ns per call, 3 or 5 calls in our code.

Testing methodology is wrong, Calling delegates, by Microsoft benchmarks, is awefully slow. Class method ~4ns, delegate ~80ns. In your code, you call the delegate on each iteration, you are probably measuring performace of delegates rather than strings. I will commit better test tomorrow, so lets for now assume your measurement is in reality say 25% of your measure, by my eyeball. Thats for no2 test, 98-61//4 => 9.25ms per 100'000 calls, and we call maybe 30 those? Do you understand HOW MANY CPU CYCLES we get out of it?... Difference is in nanoseconds. 0.002ns in TOTAL. I feel the need, need for speed!!!... :)

Sorry for those comments, I just thought those were garbage/leftovers.

[arekbulski]

Oops, made a stupid mistake in number crunching. Divided by 100'000 yields per call but still in ms. (I was going to divide it by million to get nanoseconds and messed it...). So thats 0.002775ms in total per frame, right? Thats not negligible but I would point out, its very little. And only if either window is visible, because otherwise those data are not computed at all.

[arekbulski]

My branch got messed up, I need to close this PR and resubmit.

[linuxgurugamer]

I don't like people who say they are evangelists. Not because of what they believe in, but because it tends to blind them to other solutions. If your code was working perfectly and totally bug-free, I would be more amenable, however, even your latest code isn't working. If you had looked at the log file, you would have seen the exception I posted in the other thread.

Readability isn't everything. In those cases where the code is optimized for speed, comments can be used to explain what is being done and why.

I'm going to respond in the latest PR regarding the exception, how I fixed it and the results of the timing test