Optimize loops

[NRH-AA]

Pull Request Prelude

• [x] I have followed proper The Forgotten Server code styling.
• [x] I have read and understood the contribution guidelines before making this PR.
• [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Remove redundant function calls in loops

Issues addressed:

[MillhioreBT]

I'm not very sure but I think that these special container methods are optimized by the compiler, I have not seen anywhere that it is a good practice or optimization method, but I think @ranisalt might have a quick and correct answer for this.

Note: Although it is known that in Lua it could be an optimization practice for very large tables when it comes to counting, I don't know if it is the same case in modern C++, especially for this type of containers.

[NRH-AA]

At the very least its just good practice and there are many loops already in the source code that follow it. I figured its better to make it more consistent. As for if it matters for modern C++ I am in the same boat as you. It could be that it is not redefining the size every iteration but if it isn't might as well follow best practice anyway.

[amatria]

I'm not very sure but I think that these special container methods are optimized by the compiler

They probably are. This is good practice nonetheless: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

[MillhioreBT]

Honestly you should leave that as is, you're just adding unnecessary extra logic. the compiler already does this job and much better

Here I leave you an example to close this PR: https://quick-bench.com/q/1TeRsXRYPbRehoc7AQCFj1H0snk

Even adding temporary variables to save the precalculated size is more time-consuming, it is negligible but...

Test image


And if that were not enough, here is the comparison without compiler optimizations: https://quick-bench.com/q/2ZQg5xSsugJ-KwCjbdZHQCCy2gA

[amatria]

Even adding temporary variables to save the precalculated size is more time-consuming, it is negligible but...

I am sorry, but your benchmark is wrong. Here are the correct results:

• -O3:

• No optimizations:

The version that stores the size of the vector is never slower than calling size() repeatedly.

[NRH-AA]

You are right that the compiler does try to optimize and in some cases it does match the efficiency pretty closely. It seems after a certain amount of iterations it may start to assume the size is not going to change and only checks it every once in a while to make sure it doesn't. Still its better to let it know if we know it will not change.

I also want to add that in some cases we also ask for the size inside of the loop which is another call to the function which takes up even more time.

Lastly, we already have loops in the source code that use the method I'm purposing we migrate too. The main thing I was thinking was to standardize them a little.

This is the output when running these two in cpp

[amatria]

@NRH-AA @MillhioreBT

This benchmarking code is erroneous, resulting in the compiler optimizing away all meaningful computations. Please enable the "Show Noop bar" option. Refer to my comment above for the correct benchmark.

[ranisalt]

One of these changes is not equivalent and makes the code incorrect; the rest, I think we should always write the simplest code, in that case it is keeping unchanged.

Thanks for trying, I'm sure you're learning a lot about the codebase 😉

[NRH-AA]

I probably should of looked a little closer on the loops to make sure the sizes weren't changing during them. I do want to challenge the idea of simplest code and tfs though haha. Anyway, ill keep that in mind. Just trying to find places to help. Thanks for feedback.