Skip to content
This repository was archived by the owner on Jan 6, 2022. It is now read-only.

Conversation

@Aeronaut
Copy link

I think this is a little piece faster than comparing each value two times...

@Evrey
Copy link

Evrey commented Apr 18, 2016

Too few bithacks in there! ;<

Just kidding. Nice little patch, that also keeps the code easy to read. Is this hot path code, or was it just you having fun?

@Jimmacle
Copy link

Is the only real difference combining Min and Max into one method? That might save the (small) overhead of a second method call but it seems excessive to add a whole new method to Vector3 for one use case.

@Spartan322
Copy link

Spartan322 commented Apr 18, 2016

There is a bit more overhead save then just a method call (though the cost would still be highly minor), but even then, I would think having both a min and max combined method would have a bit more need all around the source, this does seem like a rather tiny scoped improvement. If more cases can't be found, this will probably become random useless bloat.

@Aeronaut
Copy link
Author

Aeronaut commented Apr 22, 2016

From my point of view this discussion will be waste of time if half of the involved can't see the lesser overhead.
I think it's use cases aren't as relevant as the amount of calls on it.

@Spartan322
Copy link

The reason we are telling you this overhead save is not worth much time is the .NET library is already a huge overhead situation, in comparison, this save is like comparing an atom to at the least a galaxy, if not the universe.

@devu
Copy link

devu commented Apr 22, 2016

Every little counts. Few more atoms like this in crucial places and you
could gain few ms per cycle. That is not universe but certainly would make
less people winding up about performance issues.
I know there is a saying "early optimization is a source of all evil". Is
not early. It's about time to look into those issues imho.

On 22 April 2016 at 12:12, Geroge notifications@github.com wrote:

The reason we are telling you this overhead save is not worth much time is
the .NET library is already a huge overhead situation, in comparison, this
save is like comparing an atom to at the least a galaxy, if not the
universe.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#515 (comment)

@Spartan322
Copy link

Spartan322 commented Apr 22, 2016

@devu First, this has shown to have almost no use cases (so the cost of the PR is already unlikely to be worth time, if there were actually a couple use cases, then it wouldn't be such an easy decision), second, the most expensive overhead is the one less function call, which is worth nothing in a .NET context, even in C, function calls cost near nothing, unless you use a recursive function and can bypass the buffer cutoff, you will never experience an optimization effect from saving function calls.

@mze9412
Copy link

mze9412 commented Apr 22, 2016

Such a simple function might even me inlined by the .Net compiler already. Please verify with performance testing before making such a pull request :)

@Aeronaut
Copy link
Author

You all spend this much time in commenting my PR that it seems more than obvious that you can at least request a performance test and so on... But this is your logic and not mine. So you can do performance test to verify what I mean with "a little piece faster" and optimize your commenting ability by adding a argument.

@Spartan322
Copy link

Spartan322 commented Apr 23, 2016

You do realize you never gave us a performance test (I mean did you not expect such small changes to actually get this kind of feedback?), also, some of us understand how a compiler works and how it optimizes, so logic would dictate that this type of PR wouldn't play outside the bounds of that understanding, which pretty much means this PR is unlikely to have visible effects (even with a large amount of reduced function calls, still unlikely as they cost near nothing) especially with a singular use case shown. That is why a performance test should have been supplied first.

@Aeronaut
Copy link
Author

Aeronaut commented Apr 24, 2016

@Spartan322 you can be sure that I know, that I don't give you a performance test and it's nice that you think that some of the involved know how a compiler works. I just can tell you, that I'm not that smart and don't know how a compiler works, I just can imagine some or more parts.

Now there is my piece of code that obviously half's the method calls and comparisons of floats what means that this is twice as fast -- against the hypothesis that the compiler do this cross method optimizations...
Now its your logic that I have to confute the hypothesis that the compiler do this... I will follow this logic now for only one time and validate @mze9412 hypothesis.
But in future please start to validate your hypothesis by your own.

@mze9412 let me know if this is your logic too, because then I will remember that you owe me one...

Performance test: C# http://pastebin.com/4eqFy5ny

1=00:00:00.0010736 (First run with Min Max separated)
2=00:00:00.0005278 (First run with MinMax combined)
3=00:00:00.0007714 (Second run with Min Max separated)
4=00:00:00.0003624 (Second run with MinMax combined)

@mze9412
Copy link

mze9412 commented Apr 24, 2016

I looked at your results.

The second run is faster btw because the JIT compiler is getting better at predicting what you want to do and maybe even inlines the function at machine code level (hard to directly find out).
The difference between 3 and 4 is about 50%, which is expected (half number of function calls), but yeah, it's about 3-4ms for 10.000 runs.

If the game calls this stuff that often, merging it could be useful. If the call is only done <100 times per game tick, no one will notice a difference. :)

Thanks for doing the effort!

On my machine the numbers look more or less the same, but it jitters depending on what else I do. Best result I got for number 4 was 3.624ms, worst result was 7.something.

@Aeronaut
Copy link
Author

Aeronaut commented Apr 24, 2016

Thanks for doing the effort!

Your welcome!

Having it would bring more performance, that's the only thing that counts in my eyes. :-)

@Spartan322
Copy link

Spartan322 commented Apr 24, 2016

It seems this will only be worth a pull if it is called often as stated by mze, and seeing as the use cases are still slim, I'm still skeptical on whether this is worth investigating, as the addition to Vector3 is otherwise unnecessary bloat. (which we should be trying to cut, not add)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants