I permit myself to add a com (as phab sent me a O2 thing that morning).
I post that here instead of D950
Nothing critical but:

• I guess the current code was done like that (mult instead of Math.pow) for performance issue.
• We could just ignore that as we don't really take care about that kind of things in some other places (me the first).
• JsPerf report really various diff (it seems that most recent interpreter crush those diffs) https://jsperf.com/math-pow-vs-simple-multiplication
• In my own test (with many biases as time precision and things like that), I noticed that the new one distanceToSquared (in our engine, not in a browser) has 3x running time on average.

(I don't know how that works internally but the change about removing cos = Math.cos(a) doesn't seem changing perf.)

The link you sent isn't relevent because we have a custom Math.pow function. Also browsers can have a custom implementation. Testing it in a browser is more relevant to webdev.

We can just get the number microseconds and loop around a call some thousand times to test the performance.
We can even write a test that there are no performance regressions with that.

If our implementation of Math.pow is slower, then we could also just add if n=2 return x*x.
Alternatively we could also implement Math.square.
There are hundreds of occurrences like that in other places where algebraic terms are duplicated (, so it's globally relevant and there is really no reason to first write the number into a variable and then square it unless proven otherwise.

We already have a globalscripts Math test in simulation components, so I'll be back with a performance test.

lorraine_plain.js:			placer = new ClumpPlacer(floor(PI*scaleByMapSize(10,20)*scaleByMapSize(10,20)/4), 0.95, 0.6, 10, fractionToTiles(0.5 + 0.49*cos(tang)), fractionToTiles(0.5 + 0.49*sin(tang)));

This one for example would look much better as a Math.PI * Math.square(scaleByMapSize(5, 10)) than PI*scaleByMapSize(10,20)*scaleByMapSize(10,20)/4)

In rP20263#23656, @elexis wrote:

The link you sent isn't relevent because we have a custom Math.pow function. Also browsers can have a custom implementation. Testing it in a browser is more relevant to webdev.

You didn't read what I wrote.

JsPerf report really various diff (it seems that most recent interpreter crush those diffs) https://jsperf.com/math-pow-vs-simple-multiplication
In my own test (with many biases as time precision and things like that), I noticed that the new one distanceToSquared (in our engine, not in a browser) has 3x running time on average.

EDIT: I am not a great user of bold stuff, but the relevant part is " (in our engine, not in a browser)"

Mostly didn't understand your comment then. How did you test? Which function did you use to measure the time? Date.now()?

In rP20263#23659, @elexis wrote:

Mostly didn't understand your comment then. How did you test? Which function did you use to measure the time? Date.now()?

I did a custom test.
I simulated sample of 10^7 (iirc) random vectors and compute the running time of applying the distanceToSquared function to those vectors with Date.now() (around the whole sample, not on each one to avoid precision issues). I did that 100 times.

EDIT: but as said in my first comment, I guess it won't even be noticeable in a game.

EDIT: but as said in my first comment, I guess it won't even be noticeable in a game.

It most likely won't, but I fail to see the logic why we should make something that is used by quite a few things slower and not much more readable (on top of introducing a bug). Compare dx*dx to Math.pow(dx, 2). And no matter how much we try to improve our Math.pow we will always be worse than just doing a*a when we'd just do Math.pow(a, 2).

Also I'm not sure that the "need" to remove a few intermediate variables justifies possibly making things like actual collision tests slightly slower. Then again if that is indeed worth it, I'd appreciate a few numbers to make sure it doesn't get worse.

That said a few of the uses of one of distanceTo could be made faster by squaring what it is compared to (eg UnitAI via DistanceBetweenEntities).

If a Math function is slower (both on average and in the particular case n=2) than it could be,
then that should be addressed in the function, not in every caller.

I'm unifying uncountable repetitions of longTerm * longTerm in the rmgen directory ,
so a separately introduced Math.square() would have uncountable use cases already that achieve the highest performance and highest readability simultaneously.

Wheather it is possible to make Math.pow more performant for any given argument is somthing I might estimate after finishing my performance test.
Unfortunately it seems like we have to Math.orig_pow = Math.pow; in Math.js, because we can't retrieve the overwritten function with any of the Object prorotype functions I found.

But before adding a performance test to cover a bug in a test case that tests bugged vector code, I have to determine wheather it is just include or split JSInterface_Debug.cpp to get the microsecond timer.
But before I can do that, I found a bug with the cxxtest library or how we use it (#4807). (And that still doesn't address the apparently incredibly crappy gameplay)

In rP20263#23663, @elexis wrote:

If a Math function is slower (both on average and in the particular case n=2) than it could be,
then that should be addressed in the function, not in every caller.

It must be slower in that particular case, since even in the best case it has to check if it should compute the thing for n=2 and return it, and that check is more work than just doing the thing for n=2.
There is no way to make a generic function faster than that.

I'm unifying uncountable repetitions of longTerm * longTerm in the rmgen directory ,
so a separately introduced Math.square() would have uncountable use cases already that achieve the highest performance and highest readability simultaneously.

Then why was this committed instead of that square extension proposed and then used here to clean up things?

Wheather it is possible to make Math.pow more performant for any given argument is somthing I might estimate after finishing my performance test.

See above, for why it cannot attain the same performance all the time, it might once the JIT kicks in and throws away everything for the non-n=2 case, which should work until for some reason that code is then thrown out again.

Unfortunately it seems like we have to Math.orig_pow = Math.pow; in Math.js, because we can't retrieve the overwritten function with any of the Object prorotype functions I found.

It was replaced, so yes it isn't there anymore.

(#4807)

See comment there.

Also that doesn't address why we now compute cos and sin twice (yes, this will hopefully fixed by the JS engine, as it is relatively easy to do, assuming it can prove that none of those calls has any side effects), but why not keep the code in a way that we do not have to rely on it doing that?

In rP20263#23664, @leper wrote:

There is no way to make a generic function faster than that.

If we solely check for n=2 then agree that the average case is slower. But the Math.pow function already has checks to do an alternative computation function (intPow) for some values, so there might be further optimizations.

Then why was this committed instead of that square extension proposed and then used here to clean up things?

I didn't expect Math.pow to be slower than x * x. If we introduce Math.square for performance reasons, then that performance reason should be a more solid analysis than what we did so far.
Math.square might still be introduced as it might be even more readable than Math.pow(x, 2) however.

After said day of performance testing with P85:

• It seems our Math.pow implementation is slower than the native one:

94% of the int calls were too slow.
82% of the float calls were too slow.
100% of the NaN calls were too slow.

• it seems that a function doing x * x * ... * x using for-loop is faster than Math.pow / Math.intPow, so apparently SpiderMonkey needs fixing (because its not even an approximation but the correct computation). Calling Math.fasterPow(x, 2) is only about 10% slower than Math.square(x) = x * x.
• it seems that the original Math.pow is faster than the Math.exp computation for fractional numbers, so apparently we should remove that.

The test suite seems awkward, so not sure if we want that commited. It does have the potential to show wheather our custom approximation is worse than the native code though.

Readability is certainly is subjective concept, and for me, x * x is more readable than any Math.square(x) or Math.pow(x, 2) or whatever else. It's only when you want to compute the square of a lengthy expression that Math.square is usefull.
Concerning speed, here again I don't understand how any of these functions calls can be faster than a simple x * x as they will eventually do it (after some additional function calls and checks).

Micro-optimization is typically a waste of time. Even if x*x is significantly faster than Math.pow(x,2), changing it won't result in a noticeable performance improvement for the game as a whole.

In rP20263#23670, @Nescio wrote:

Micro-optimization is typically a waste of time. Even if x*x is significantly faster than Math.pow(x,2), changing it won't result in a noticeable performance improvement for the game as a whole.

Saying that without any facts supporting that, is ambitious.
How can you know it's not heavily used in map generation?

About the performance, a single sin or pow call is irrelevant. But calling them a million times in a row as is done on random map scripts is highly relevant and can reduce the loading screen time by 10s of seconds.
(One particular example is the bugfix that was applied to only one of the copies of the Unknown maps in rP14162 is still present in the other Unknown maps and can cause a 3 minute + loading screen time for no reason).
Also thousands of units on the map can call that function frequently in the simulation. So if we want to increase the population limit, we have to take care of everything, not like I did in this commit.

I agree that readability as a whole is an arbitrary concept. But aspects thereof can be intersubjectively true and thus can benefit everyone.
There are many studies investigating how text layout can improve readability or reading speed and we can observe newspapers being styled the way they are for that reason.
Also I believe esoteric programming language would be intersubjectively rejected due to unreadability.

Four intersubjective disadvantages of unneeded helper variables:

• The reader reads code top to bottom. So they first encounter helper variables before seeing the place they are used in. So the eyes have to jump back and forth iteratively instead of being able to read the code in one consecuntive go.
• If there are many preliminary helper variables, it is harder to find unused ones in between.
• The reader has to judge wheather the variable name is correct.
• The reader has to ensure that the same variable is not refered to anywhere below or above unintentionally.

These points are not applicable to this commit, but if we talk about several thousand of occurrences, this becomes highly relevant.

These points are not applicable to this commit, but if we talk about several thousand of occurrences, this becomes highly relevant.

And nobody here is disagreeing with fixing those cases, but in this specific case here there is no real advantage, but an arguably small disadvantage. So at least I'd be for reverting the Math.pow parts (and fixing that one bug).