JoeyDeVries / Cell

OpenGL C++ Graphics Engine
Other
915 stars 124 forks source link

Various changes to math and utility libraries. #1

Closed htmlboss closed 7 years ago

htmlboss commented 7 years ago

As promised I didn't change anything that could break any usages (I hope I didn't anyways πŸ˜„) in the cell project (just underlying stuff).

I didn't change the matrix class to use std::array because it will involve some nasty typedefs (something like std::array< std::array<T, m>, n> data and I think it will just over-complicate a bunch of things.

Everything appears to have been working normally when I built all the libs and ran the math test program.

Let me know if anything needs fixing πŸ˜†!

JoeyDeVries commented 7 years ago

Hey Nabil! Thank you! I will definitely check these out! I was a bit late in response due to holiday/Christmas so I'll probably take a look at the code a few days later.

I have a few questions (mostly out of interest as I'm not a C++ expert and I'd love to know more):

(e.g. for (auto& it : data); guarenteed to give best speed).

Doesn't (auto& it : data) get translated to iterators behind the scenes anyways? Likely resulting in the same compiler code and thus the same performance?

Replaced unsigned int with std::size_t (it basically means the same thing but less characters to type).

On 64 bit machines std::size_t will default to a 64 bit unsigned integer, but I beleive unsigned int still defaults to a 32 bit integer. In that case it might not be beneficial to replace all unsigned integers with size_t right? (for loops it probably wouldn't make a difference anyways), but for state data it might be a disadvantage for cache efficiency.

I'll check out the code later this week :) And thank you for indeed not making the code too complicated as I do prefer readability over correctness and (small-level) efficiency.

Thank you! and happy holidays!

htmlboss commented 7 years ago

Warning: Wall of Text :rofl:

Doesn't (auto& it : data) get translated to iterators behind the scenes anyways? Likely resulting in the same compiler code and thus the same performance?

Yes it should! As of C++17 the range-based loop would decompose to:

{
    auto && __range = range_expression ; 
    auto __begin = begin_expr ;
    auto __end = end_expr ;
    for ( ; __begin != __end; ++__begin) { 
        range_declaration = *__begin; 
        loop_statement 
    } 
}

So, technically they can be the same thing, but in general:

Range loops are for cases when we are interested in examining every item in a collection (unless we break out sooner) and aren't interested in doing anything with the container itself. Like it was already stated, range loops are optimized that way, e.g. it indeed computes the finishing condition only once. In my opinion, this is a very nice and welcome addition to the variety of loop options in c++, since we do often face this very exact situation: we get a container and are interested in simply going through them one by one in either const or non-const fashion. (http://stackoverflow.com/a/29578982/2231969)

So, it's kind of a convention thing but it's also less code needed to achieve the same thing (in this case).


On 64 bit machines std::size_t will default to a 64 bit unsigned integer, but I beleive unsigned int still defaults to a 32 bit integer. In that case it might not be beneficial to replace all unsigned integers with size_t right? (for loops it probably wouldn't make a difference anyways), but for state data it might be a disadvantage for cache efficiency.

Hmmm...unsigned int CAN take on various sizes depending on platform (cuz Microsoft :man_facepalming: ), compiler/compiler optimizations, and architecture. You've got a good point with the cache efficiency statement. The thing about size_t is that it will be able to hold the largest size object supported by the targeted platform (x86, x64). The weird thing about it is that I can't find any accepted best practices for when or not to use it. I simply defaulted to it since the STL uses it very liberally. However:

A good rule of thumb is for anything that you need to compare in the loop condition against something that is naturally a std::size_t itself. std::size_t is the type of any sizeof expression and as is guaranteed to be able to express the maximum size of any object (including any array) in C++. By extension it is also guaranteed to be big enough for any array index so it is a natural type for a loop by index over an array. If you are just counting up to a number then it may be more natural to use either the type of the variable that holds that number or an int or unsigned int (if large enough) as these should be a natural size for the machine.

But it is also worth noting that security issues may arise if not used in some cases: http://stackoverflow.com/a/3261019/2231969

Soooooo...would only using size_t in for-loops and keep unsigned int for templates make sense? It's kind of a situational thing whether to use it or not 😁


Happy Holidays too!

JoeyDeVries commented 7 years ago

Very interesting, thank you for the writeup!

Hmmm...unsigned int CAN take on various sizes depending on platform

A potential solution is to use the cstdint header and typecast their uint32_t type to something like uint32 and use that one consistently. However, seeing as for most cases size_t has the advantage over the unsigned integer I'll probably focus on using it more frequently as well. Indeed for cases where cache efficiency does become relevant it might be worth considering fixed-length types (in which case I should use the cstdint anyways).

I'll try and find some time to work through your code this week :)

JoeyDeVries commented 7 years ago

I've reviewed the items and merged them into the repository, thank you!

I had some questions/remarks though :)

Again, thank you for the effort! I hope to continue development on the engine again once the PBR tutorials are finished :)

htmlboss commented 7 years ago
  1. This is actually a very opinion-based question 😁. My general convention is:
    • If a parameter is not being altered, it's passed as const (as a guarantee to the client programmer that we're not changing it).
    • If a parameter is a primitive type, pass-by-value.
    • If a parameter is a structure (or some complex type), pass-by-reference.
    • If we have a template parameter, pass-by-reference, unless we put in std::is_integral<T> to guarantee T is a primitive (which might be something we could incorporate later into the specialized Vector and Matrix classes).

The main thing about passing-by-value is that a copy of the parameter is made (so memory can become an issue). Here are some opinions: https://stackoverflow.com/questions/25273958/when-is-an-object-sufficiently-large-that-there-is-a-performance-gain-in-passing?noredirect=1&lq=1 .

Bottom line is this (and also is applicable for #2 below): profiling doesn't lie 😁. Just keep these questions you have in mind and when the engine is sufficiently complex, you can run some tests for each configuration (then we can revert some things if necessary). The course I usually take is to make my program as "safe" as possible, then run a bunch of tests to see what's going on.

  1. There is a performance difference between operator[] and ::at() (it could be rather large depending how aggressively you are optimizing your Release build). But assert() is reeaaly more targeted toward making debugging easier and isn't the best option for production-standard error handling by itself: https://stackoverflow.com/questions/17732/when-should-assertions-stay-in-production-code .

The reason is that an unchecked access can probably be done with a single processor instruction. A checked access will also have to load the size from memory, compare it with the index, and (assuming it's in range) skip over a conditional branch to the error handler. There may be more faffing around to handle the possibility of throwing an exception. This will be many times slower, and this is precisely why you have both options.

If you can prove that the index is within range without a runtime check then use operator[]. Otherwise, use at(), or add your own check before access. operator[] should be more or less as fast as possible, but will explode messily if the index is invalid.

So, I'll keep looking around off-and-on to see if there are any potential solutions that could be integrated here.

  1. math.h is the old C-library, while <cmath> (or anything that has the form <c...>) is/are the C++ standard headers. The only real differences are that all of the functions are encapsulated in the std namespace in <cmath> (as to prevent name pollution), and have been adjusted to ensure static type-safety.

Can't wait for the new PBR tutorials!! πŸ˜‹

JoeyDeVries commented 7 years ago

profiling doesn't lie

I 110% agree! Let's just do some major profiling rounds once I've added most of the major points of the engine around some simple/complex demo scenes to see what difference it 'll make. That can definitely be interesting! :)

There is a performance difference between operator[] and ::at() (it could be rather large depending how aggressively you are optimizing your Release build)

Do you have any data for this? I can't imagine anything that could be faster than [] (or then how the compiler can better optimize code if .at() is used?

But assert() is reeaaly more targeted toward making debugging easier and isn't the best option for production-standard error handling

I agree, it's not really there for error handling. However, if user code fails to do proper bound checks the application will crash with/without the assert (which I'm fine with as this is a bug), I keep the assert there to indeed make it easier to test assumptions during debugging (I probably disable their use with preprocessor directive in release).

I'm not sure of any other alternatives, but probably a safer error handling would be to use exceptions then (which I believe .at() throws if I'm not mistaken). However, I'm still not really sold on the use of exceptions in high-performant code as I don't think the benefits outweigh the negatives in this context (which is why I want to disable exception support when building; not sure if I already did that). But again, this could be a case for profiling.

math.h is the old C-library,

That's good to know! I'll try to refrain from using it and use some C++ varirant wherever I can.

Thank you! :)

htmlboss commented 7 years ago

Sooo sorry for the late response! The new semester started and Vector Calc and ODE's have me running for the hills πŸ˜›

To clarify about the speed of operator[] vs ::at(), the latter CAN be slower because of the exception handling built-in.

Nowadays, exceptions in C++ typically follow the "Zero Cost Model" which basically means that instead of setting up a guard wherever an exception can happen, a lookup table is generated that maps any possible exceptional point to a list of handlers (like custom exception handlers we can write). If we get no exceptions thrown, we lose no performance (hence the "Zero Cost" part). Conversely, when an exception is thrown, the CPU has to load the lookup table first (super slow) and get the proper handler from it (via RTTI which involves a ton of arbitrary dynamic_cast's). On average, this can cost 10-20 times that of an if() statement.

Simply: things are much slower only on the exceptional path. This then brings up the discussion that if we program intelligently, we could avoid exceptions unless something truly exceptional occurs that is out of our control πŸ˜†

JoeyDeVries commented 7 years ago

Not at all, I know what it's like to be busy with way too much stuff :stuck_out_tongue_winking_eye:

And interesting, I had no idea about the "Zero Cost Model", I'll be sure to look that up, thank you!