1. Steve Streeting
  2. ogre
  3. Pull requests

Pull requests

#711 Merged at 10d6b07
Repository
Ogre_Landmark
Branch
v2-1
Repository
ogre
Branch
v2-1

Container Optimisations

Author
  1. Matthew Fletcher
Reviewers
Description

Hi.

During testing I was loading 60k meshes with BT_DEFAULT. When trying to destroy all these meshes ogre, running in release, took over 15 minutes of doing nothing else. I say over because i terminated the process at 15 minutes. There had not yet been a noticeable drop in memory.

Profiling revealed the issue was O(n) lookups in a number of vectors and a list to erase the correct element. these were in the VaoManager and the ResourceGroupManager. I switched these to use unordered_set and deleting 60k meshes now takes about 4 seconds, a non-trivial difference!

The places using vector don't actually appear to be frequently iterating so switching should not have a noticeable impact in other areas.

In this change:

  • I replaced the OGRE_HashMap macro with a templated type like other containers. This also means the unordered containers will not also be able to use the custom allocator.

  • replaced some usage of vector with unordered_set

  • Added a hash specialisation for SharedPtr so it may be used in unordered containers

I'm running VS 2015 here and this change works great. I have not been able to test other compilers but made sure to read the docs for them and expect the templated unordered stuff should compile on them also.

Comments (9)

  1. Matias Goldberg

    (⌐■_■) ( ಠ_ಠ)>⌐■-■ ಠ_ಠ

    +15 minutes down to 4 seconds??? Dear mother of God!

    I would like to test this on other compilers, but I cannot checkout your repository because it's private and I don't have access. I suggest two solutions:

    1. Add me to your forked repo with read access (that's all I need).
    2. If there's also other proprietary changes you don't want me to see and thus you can't add me; create another fork, and upload your changes there and give me access (use hg push -r 3001 to push a specific revision 3001 and its ancestors)

    And regarding the VaoManager's vector: Yes, it's only purpose is to keep track of allocations (rather than iterating). I chose a vector to keep memory contiguous rather than sparse (e.g. std::set fragments a lot) and figured out the lookups wouldn't be too bad. But obviously looks like I was wrong.

    Good Job.

  2. Matthew Fletcher author

    I added you to the repo. Nothing in here is secret it's only a private fork to make pull requests simple for me :). I'll leave you with access in case you want to look at something for any future requests.

  3. Matias Goldberg

    I've been testing. We don't use C++11, so far:

    1. VS2008 will trigger a compiler error on template < typename T > struct hash< Ogre::SharedPtr< T > > : hash< T* > which is solved by adding it inside the tr1 space. Everything works afterwards.
    2. Clang 3.9 on Linux will complain about the same thing, but with a helpful suggestion of using the tr1 namespace. But run crashed. I'm still researching. Update: The crash has nothing to do with this PR.
    3. GCC 5.4 also needs the hash to be inside tr1 namespace.

    I know need to test VS 2015 when using the tr1 namespace.

  4. Matthew Fletcher author

    Ah hash is in tr1 also. I probably should have seen that coming. I can adjust the path to fix that up, or alternatively i can grant you write access to the fork so you can push any adjustments directly to the PR?

  5. Matthew Fletcher author

    Hi!

    I just finished watching netflix and was about to go to bed when i saw this. it looks to me like its an include problem, as in boost isnt included before the prerequisite header.

    Moving OgreStdHeaders.h above OgrePrerequisites.h in OgreStableHeaders.h should fix that, if that is the case. I also noticed the #define guards in OgreStdHeaders.h now does not match the guard in OgrePrerequisites that i changed slightly. They should probably both be changed to match the method you used to handle the hash change. It could also be extended for earlier VC++ versions that also had a hash_map... i wouldnt bother, but I'm not sure what kind of old compilers ogre wants to support.

    Bed time!

  6. Matias Goldberg

    OgreStdHeaders.h is already included inside OgrePrerequisites.h

    I think I know what's going on. This is OgreStdHeaders.h

    #   if (OGRE_COMPILER == OGRE_COMPILER_MSVC) && _MSC_FULL_VER >= 150030729 // VC++ 9.0 SP1+
    #       include <unordered_map>
    #       include <unordered_set>
    #   elif OGRE_THREAD_PROVIDER == 1
    #       include <boost/unordered_map.hpp>
    #       include <boost/unordered_set.hpp>
    #   else
    #       error "Your compiler doesn't support unordered_set and unordered_map. Try to compile Ogre with Boost or STLPort."
    #   endif
    

    150030729 corresponds to Visual Studio 2008 SP1, but OgrePrerequisites:

    #   if OGRE_COMPILER == OGRE_COMPILER_MSVC && !defined(_STLP_MSVC)
    #       define OGRE_HASHMAP_NAME unordered_map
    #       define OGRE_HASHMULTIMAP_NAME unordered_multimap
    #       define OGRE_HASHSET_NAME unordered_set
    #       define OGRE_HASHMULTISET_NAME unordered_multiset
    #       if _MSC_VER >= 1900 // VC++ 2015
    #           define OGRE_HASH_NAMESPACE ::std
    #       elif OGRE_THREAD_PROVIDER == 1
    #           define OGRE_HASH_NAMESPACE ::boost
    #       else
    #           define OGRE_HASH_NAMESPACE ::std::tr1
    #       endif
    #   else
    

    If I'm reading this right, boost headers for unordered_* will only be included in pre-VS2008, but when OGRE_THREAD_PROVIDER == 1 boost namespace will be used for every VS compiler except VS2015.

    Btw. VS 2008 is the oldest compiler we're supporting.