Merge branch 'feature/optimize-paged-volume' into develop

CMakeLists.txt

@@ -37,6 +37,10 @@ IF(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") #Maybe "OR
 	ADD_DEFINITIONS(-std=c++0x) #Enable C++0x mode
 ENDIF()
 
+IF(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
+	ADD_DEFINITIONS(-D_CRT_SECURE_NO_WARNINGS)
+ENDIF()
+
 ADD_SUBDIRECTORY(include)
 
 OPTION(ENABLE_EXAMPLES "Should the examples be built" ON)

examples/OpenGL/Shapes.cpp

@@ -57,8 +57,8 @@ void createSphereInVolume(RawVolume<MaterialDensityPair88>& volData, float fRadi
 
 void createCubeInVolume(RawVolume<MaterialDensityPair88>& volData, Vector3DInt32 lowerCorner, Vector3DInt32 upperCorner, uint8_t uValue)
 {
-	uint8_t maxDen = MaterialDensityPair88::getMaxDensity();
+	uint8_t maxDen = static_cast<uint8_t>(MaterialDensityPair88::getMaxDensity());
-	uint8_t minDen = MaterialDensityPair88::getMinDensity();
+	uint8_t minDen = static_cast<uint8_t>(MaterialDensityPair88::getMinDensity());
 	//This three-level for loop iterates over every voxel between the specified corners
 	for (int z = lowerCorner.getZ(); z <= upperCorner.getZ(); z++)
 	{

include/PolyVox/PagedVolume.h

@@ -280,33 +280,23 @@ namespace PolyVox
 		/// Assignment operator
 		PagedVolume& operator=(const PagedVolume& rhs);
 
-	private:	
+	private:
-		std::shared_ptr<Chunk> getChunk(int32_t uChunkX, int32_t uChunkY, int32_t uChunkZ) const;
+		bool canReuseLastAccessedChunk(int32_t iChunkX, int32_t iChunkY, int32_t iChunkZ) const;
+		Chunk* getChunk(int32_t uChunkX, int32_t uChunkY, int32_t uChunkZ) const;
 
-		void purgeNullPtrsFromAllChunks(void) const;
+		// Storing these properties individually has proved to be faster than keeping
+		// them in a Vector3DInt32 as it avoids constructions and comparison overheads.
+		// They are also at the start of the class in the hope that they will be pulled
+		// into cache - I've got no idea if this actually makes a difference.
+		mutable int32_t m_v3dLastAccessedChunkX = 0;
+		mutable int32_t m_v3dLastAccessedChunkY = 0;
+		mutable int32_t m_v3dLastAccessedChunkZ = 0;
+		mutable Chunk* m_pLastAccessedChunk = nullptr;
 
-		// The chunk data is stored in the pair of maps below. This will often hold the same set of chunks but there are occasions
+		mutable std::unordered_map<Vector3DInt32, std::unique_ptr< Chunk > > m_mapChunks;
-		// when they can differ (more on that in a moment). The main store is the set of 'recently used chunks' which holds shared_ptr's
-		// to the chunk data. When memory is low chunks can be removed from this list and, assuming there are no more references to
-		// them, they will be deleted. However, it is also possible that a Sampler is pointing at a given chunk, and in this case the
-		// reference counting will ensure that the chunk survives until the sampler has finished with it.
-		//
-		// However, this leaves us open to a subtle bug. If a chunk is removed from the recently used set, continues to exist due to a
-		// sampler using it, and then the user tries to access it through the volume interface, then the volume will page the chunk
-		// back in (not knowing the sampler still has it). This would result in two chunks in memory with the same position is space.
-		// To avoid this, the 'all chunks' set tracks chunks with are used by the volume *and/or* the samplers. It holds weak pointers
-		// so does not cause chunks to persist.
-		//
-		// TODO: Do we really need maps here? It means we are storing the chunk positions in the map, but they are also stored in the
-		// chunks themselves (so they can be passed to the pager from the chunks destructor). Can we use a set here? What is a better approach?
-		typedef std::unordered_map<Vector3DInt32, std::weak_ptr< Chunk > > WeakPtrChunkMap;
-		mutable WeakPtrChunkMap m_pAllChunks;
-		typedef std::unordered_map<Vector3DInt32, std::shared_ptr< Chunk > > SharedPtrChunkMap;
-		mutable SharedPtrChunkMap m_pRecentlyUsedChunks;
 
 		mutable uint32_t m_uTimestamper = 0;
-		mutable Vector3DInt32 m_v3dLastAccessedChunkPos = Vector3DInt32(0, 0, 0); //There are no invalid positions, but initially the m_pLastAccessedChunk pointer will be null
+
-		mutable std::shared_ptr<Chunk> m_pLastAccessedChunk = nullptr;
 		uint32_t m_uChunkCountLimit = 0;
 
 		// The size of the volume

include/PolyVox/PagedVolume.inl

@@ -123,7 +123,8 @@ namespace PolyVox
 		const uint16_t yOffset = static_cast<uint16_t>(uYPos & m_iChunkMask);
 		const uint16_t zOffset = static_cast<uint16_t>(uZPos & m_iChunkMask);
 
-		auto pChunk = getChunk(chunkX, chunkY, chunkZ);
+		auto pChunk = canReuseLastAccessedChunk(chunkX, chunkY, chunkZ) ? m_pLastAccessedChunk : getChunk(chunkX, chunkY, chunkZ);
+
 		return pChunk->getVoxel(xOffset, yOffset, zOffset);
 	}
 
@@ -155,7 +156,8 @@ namespace PolyVox
 		const uint16_t yOffset = static_cast<uint16_t>(uYPos - (chunkY << m_uChunkSideLengthPower));
 		const uint16_t zOffset = static_cast<uint16_t>(uZPos - (chunkZ << m_uChunkSideLengthPower));
 
-		auto pChunk = getChunk(chunkX, chunkY, chunkZ);
+		auto pChunk = canReuseLastAccessedChunk(chunkX, chunkY, chunkZ) ? m_pLastAccessedChunk : getChunk(chunkX, chunkY, chunkZ);
+
 		pChunk->setVoxel(xOffset, yOffset, zOffset, tValue);
 	}
 
@@ -214,17 +216,11 @@ namespace PolyVox
 	template <typename VoxelType>
 	void PagedVolume<VoxelType>::flushAll()
 	{
-		// Clear this pointer so it doesn't hang on to any chunks.
+		// Clear this pointer as all chunks are about to be removed.
 		m_pLastAccessedChunk = nullptr;
 
 		// Erase all the most recently used chunks.
-		m_pRecentlyUsedChunks.clear();
+		m_mapChunks.clear();
-
-		// Remove deleted chunks from the list of all loaded chunks.
-		purgeNullPtrsFromAllChunks();
-
-		// If there are still some chunks left then this is a cause for concern. Perhaps samplers are holding on to them?
-		POLYVOX_LOG_WARNING_IF(m_pAllChunks.size() > 0, "Chunks still exist after performing flushAll()! Perhaps you have samplers attached?");
 	}
 
 	////////////////////////////////////////////////////////////////////////////////
@@ -233,7 +229,7 @@ namespace PolyVox
 	template <typename VoxelType>
 	void PagedVolume<VoxelType>::flush(Region regFlush)
 	{
-		// Clear this pointer so it doesn't hang on to any chunks.
+		// Clear this pointer in case the chunk it points at is flushed.
 		m_pLastAccessedChunk = nullptr;
 
 		// Convert the start and end positions into chunk space coordinates
@@ -256,79 +252,52 @@ namespace PolyVox
 			{
 				for(int32_t z = v3dStart.getZ(); z <= v3dEnd.getZ(); z++)
 				{
-					m_pRecentlyUsedChunks.erase(Vector3DInt32(x, y, z));
+					m_mapChunks.erase(Vector3DInt32(x, y, z));
 				}
 			}
 		}
-
-		m_pLastAccessedChunk = nullptr;
-
-		// We might now have so null pointers in the 'all chunks' list so clean them up.
-		purgeNullPtrsFromAllChunks();
 	}
 
 	template <typename VoxelType>
-	std::shared_ptr<typename PagedVolume<VoxelType>::Chunk> PagedVolume<VoxelType>::getChunk(int32_t uChunkX, int32_t uChunkY, int32_t uChunkZ) const
+	bool PagedVolume<VoxelType>::canReuseLastAccessedChunk(int32_t iChunkX, int32_t iChunkY, int32_t iChunkZ) const
+	{
+		return ((iChunkX == m_v3dLastAccessedChunkX) &&
+			(iChunkY == m_v3dLastAccessedChunkY) &&
+			(iChunkZ == m_v3dLastAccessedChunkZ) &&
+			(m_pLastAccessedChunk));
+	}
+
+	template <typename VoxelType>
+	typename PagedVolume<VoxelType>::Chunk* PagedVolume<VoxelType>::getChunk(int32_t uChunkX, int32_t uChunkY, int32_t uChunkZ) const
 	{
 		Vector3DInt32 v3dChunkPos(uChunkX, uChunkY, uChunkZ);
 
-		//Check if we have the same chunk as last time, if so there's no need to even update
-		//the time stamp. If we updated it everytime then that would be every time we touched
-		//a voxel, which would overflow a uint32_t and require us to use a uint64_t instead.
-		//This check should also provide a significant speed boost as usually it is true.
-		if((v3dChunkPos == m_v3dLastAccessedChunkPos) && (m_pLastAccessedChunk != 0))
-		{
-			return m_pLastAccessedChunk;
-		}
-
 		// The chunk was not the same as last time, but we can now hope it is in the set of most recently used chunks.
-		std::shared_ptr<typename PagedVolume<VoxelType>::Chunk> pChunk = nullptr;
+		Chunk* pChunk = nullptr;
-		typename SharedPtrChunkMap::iterator itChunk = m_pRecentlyUsedChunks.find(v3dChunkPos);
+		auto itChunk = m_mapChunks.find(v3dChunkPos);
 
 		// Check whether the chunk was found.
-		if ((itChunk) != m_pRecentlyUsedChunks.end())
+		if ((itChunk) != m_mapChunks.end())
 		{
 			// The chunk was found so we can use it.
-			pChunk = itChunk->second;		
+			pChunk = itChunk->second.get();		
 			POLYVOX_ASSERT(pChunk, "Recent chunk list shold never contain a null pointer.");
 		}
 
-		if (!pChunk)
-		{
-			// Although it's not in our recently use chunks, there's some (slim) chance that it
-			// exists in the list of all loaded chunks, because a sampler may be holding on to it.
-			typename WeakPtrChunkMap::iterator itWeakChunk = m_pAllChunks.find(v3dChunkPos);
-			if (itWeakChunk != m_pAllChunks.end())
-			{
-				// We've found an entry in the 'all chunks' list, but it can be null. This happens if a sampler was the
-				// last thing to be keeping it alive and then the sampler let it go. In this case we remove it from the
-				// list, and it will get added again soon when we page it in and fill it with valid data.
-				if (itWeakChunk->second.expired())
-				{
-					m_pAllChunks.erase(itWeakChunk);
-				}
-				else
-				{
-					// The chunk is valid. We know it's not in the recently used list (we checked earlier) so it should be added.
-					pChunk = std::shared_ptr< PagedVolume<VoxelType>::Chunk >(itWeakChunk->second);
-					m_pRecentlyUsedChunks.insert(std::make_pair(v3dChunkPos, pChunk));
-				}
-			}
-		}
-
 		// If we still haven't found the chunk then it's time to create a new one and page it in from disk.
 		if (!pChunk)
 		{
 			// The chunk was not found so we will create a new one.
-			pChunk = std::make_shared< PagedVolume<VoxelType>::Chunk >(v3dChunkPos, m_uChunkSideLength, m_pPager);
+			pChunk = new PagedVolume<VoxelType>::Chunk(v3dChunkPos, m_uChunkSideLength, m_pPager);
+			pChunk->m_uChunkLastAccessed = ++m_uTimestamper; // Important, as we may soon delete the oldest chunk
+			m_mapChunks.insert(std::make_pair(v3dChunkPos, std::unique_ptr<Chunk>(pChunk)));
 
 			// As we are loading a new chunk we should try to ensure we don't go over our target memory usage.
-			bool erasedChunk = false;
+			while (m_mapChunks.size() > m_uChunkCountLimit)
-			while (m_pRecentlyUsedChunks.size() + 1 > m_uChunkCountLimit) // +1 ready for new chunk we will add next.
 			{
 				// Find the least recently used chunk. Hopefully this isn't too slow.
-				typename SharedPtrChunkMap::iterator itUnloadChunk = m_pRecentlyUsedChunks.begin();
+				auto itUnloadChunk = m_mapChunks.begin();
-				for (typename SharedPtrChunkMap::iterator i = m_pRecentlyUsedChunks.begin(); i != m_pRecentlyUsedChunks.end(); i++)
+				for (auto i = m_mapChunks.begin(); i != m_mapChunks.end(); i++)
 				{
 					if (i->second->m_uChunkLastAccessed < itUnloadChunk->second->m_uChunkLastAccessed)
 					{
@@ -337,25 +306,15 @@ namespace PolyVox
 				}
 
 				// Erase the least recently used chunk
-				m_pRecentlyUsedChunks.erase(itUnloadChunk);
+				m_mapChunks.erase(itUnloadChunk);
-				erasedChunk = true;
 			}
-
-			// If we've deleted any chunks from the recently used list then this
-			// seems like a good place to purge the 'all chunks' list as well.
-			if (erasedChunk)
-			{
-				purgeNullPtrsFromAllChunks();
-			}
-
-			// Add our new chunk to the maps.
-			m_pAllChunks.insert(std::make_pair(v3dChunkPos, pChunk));
-			m_pRecentlyUsedChunks.insert(std::make_pair(v3dChunkPos, pChunk));
 		}
-
+				
-		pChunk->m_uChunkLastAccessed = ++m_uTimestamper;
 		m_pLastAccessedChunk = pChunk;
-		m_v3dLastAccessedChunkPos = v3dChunkPos;
+		//m_v3dLastAccessedChunkPos = v3dChunkPos;
+		m_v3dLastAccessedChunkX = uChunkX;
+		m_v3dLastAccessedChunkY = uChunkY;
+		m_v3dLastAccessedChunkZ = uChunkZ;
 
 		return pChunk;	
 	}
@@ -366,28 +325,9 @@ namespace PolyVox
 	template <typename VoxelType>
 	uint32_t PagedVolume<VoxelType>::calculateSizeInBytes(void)
 	{
-		// Purge null chunks so we know that all chunks are used.
-		purgeNullPtrsFromAllChunks();
-
 		// Note: We disregard the size of the other class members as they are likely to be very small compared to the size of the
 		// allocated voxel data. This also keeps the reported size as a power of two, which makes other memory calculations easier.
-		return PagedVolume<VoxelType>::Chunk::calculateSizeInBytes(m_uChunkSideLength) * m_pAllChunks.size();
+		return PagedVolume<VoxelType>::Chunk::calculateSizeInBytes(m_uChunkSideLength) * m_mapChunks.size();
-	}
-
-	template <typename VoxelType>
-	void PagedVolume<VoxelType>::purgeNullPtrsFromAllChunks(void) const
-	{
-		for (auto chunkIter = m_pAllChunks.begin(); chunkIter != m_pAllChunks.end();)
-		{
-			if (chunkIter->second.expired())
-			{
-				chunkIter = m_pAllChunks.erase(chunkIter);
-			}
-			else
-			{
-				chunkIter++;
-			}
-		}
 	}
 }
 

include/PolyVox/PagedVolumeSampler.inl

@@ -110,7 +110,8 @@ namespace PolyVox
 			uYPosInChunk * this->mVolume->m_uChunkSideLength +
 			uZPosInChunk * this->mVolume->m_uChunkSideLength * this->mVolume->m_uChunkSideLength;
 
-		auto pCurrentChunk = this->mVolume->getChunk(uXChunk, uYChunk, uZChunk);
+		auto pCurrentChunk = this->mVolume->canReuseLastAccessedChunk(uXChunk, uYChunk, uZChunk) ?
+			this->mVolume->m_pLastAccessedChunk : this->mVolume->getChunk(uXChunk, uYChunk, uZChunk);
 
 		mCurrentVoxel = pCurrentChunk->m_tData + uVoxelIndexInChunk;
 	}

include/PolyVox/Vector.h

@@ -29,10 +29,12 @@ freely, subject to the following restrictions:
 
 #include "PolyVoxForwardDeclarations.h"
 
+#include <algorithm>
 #include <cmath>
 #include <cstring>
 #include <functional>
 #include <iostream>
+#include <memory>
 
 namespace PolyVox
 {

include/PolyVox/Vector.inl

@@ -39,10 +39,7 @@ namespace PolyVox
     template <uint32_t Size,typename StorageType, typename OperationType>
 	Vector<Size,StorageType,OperationType>::Vector(StorageType tFillValue)
     {
-		for(uint32_t ct = 0; ct < Size; ct++)
+		std::fill(m_tElements, m_tElements + Size, tFillValue);
-		{
-			m_tElements[ct] = tFillValue;
-		}
     }
 
     /**

tests/TestVolumeSubclass.cpp

@@ -73,7 +73,9 @@ public:
 	/// Gets a voxel at the position given by <tt>x,y,z</tt> coordinates
 	VoxelType getVoxel(int32_t uXPos, int32_t uYPos, int32_t uZPos) const
 	{
-		if ((uXPos < mVolumeData.getDimension(0)) && (uYPos < mVolumeData.getDimension(1)) && (uZPos < mVolumeData.getDimension(2)))
+		if ((uXPos >= 0) && (uXPos < static_cast<int32_t>(mVolumeData.getDimension(0))) &&
+			(uYPos >= 0) && (uYPos < static_cast<int32_t>(mVolumeData.getDimension(1))) &&
+			(uZPos >= 0) && (uZPos < static_cast<int32_t>(mVolumeData.getDimension(2))))
 		{
 			return mVolumeData(uXPos, uYPos, uZPos);
 		}
@@ -94,7 +96,9 @@ public:
 	/// Sets the voxel at the position given by <tt>x,y,z</tt> coordinates
 	bool setVoxel(int32_t uXPos, int32_t uYPos, int32_t uZPos, VoxelType tValue)
 	{
-		if ((uXPos < mVolumeData.getDimension(0)) && (uYPos < mVolumeData.getDimension(1)) && (uZPos < mVolumeData.getDimension(2)))
+		if( (uXPos >= 0) && (uXPos < static_cast<int32_t>(mVolumeData.getDimension(0))) &&
+			(uYPos >= 0) && (uYPos < static_cast<int32_t>(mVolumeData.getDimension(1))) &&
+			(uZPos >= 0) && (uZPos < static_cast<int32_t>(mVolumeData.getDimension(2))))
 		{
 			mVolumeData(uXPos, uYPos, uZPos) = tValue;
 			return true;