Some performance optimizations (#5424)

* Some performance optimizations This is globally removing some memory useless copy * use a const ref return on std::string Settings::get to prevent data copy on getters which doesn't need to copy it * pass some stack created strings to static const as they are not modified anywhere * Camera: return nametags per const ref instead of a list pointer, we only need to read it * INodeDefManager: getAll should be a result ref writer instead of a return copy * INodeDefManager: getAlias should return a const std::string ref * Minimap: unroll a Scolor creation in blitMinimapPixersToImageRadar to prvent many variable construct/destruct which are unneeded (we rewrite the content in the loop) * CNodeDefManager::updateAliases: prevent a idef getall copy * Profiler: constness * rollback_interface: create real_name later, and use const ref * MapBlockMesh updateFastFaceRow: unroll TileSpec next_tile, which has a cost of 1.8% CPU due to variable allocation/destruction, * MapBlockMesh updateFastFaceRow: copy next_tile to tile only if it's a different tilespec * MapBlockMesh updateFastFaceRow: use memcpy to copy next_lights to lights to do it in a single cpu operation
minetest · Mar 22, 2017 · 072bbba · 072bbba
1 parent 9efc5da
commit 072bbba
Show file tree

Hide file tree

Showing 16 changed files with 79 additions and 134 deletions.
diff --git a/src/camera.h b/src/camera.h
@@ -172,8 +172,7 @@ class Camera
 
 	void removeNametag(Nametag *nametag);
 
-	std::list<Nametag *> *getNametags()
-	{ return &m_nametags; }
+	const std::list<Nametag *> &getNametags() { return m_nametags; }
 
 	void drawNametags();
 

diff --git a/src/client/clientlauncher.cpp b/src/client/clientlauncher.cpp
@@ -530,7 +530,7 @@ bool ClientLauncher::create_engine_device()
 
 	// Determine driver
 	video::E_DRIVER_TYPE driverType = video::EDT_OPENGL;
-	std::string driverstring = g_settings->get("video_driver");
+	const std::string &driverstring = g_settings->get("video_driver");
 	std::vector<video::E_DRIVER_TYPE> drivers
 		= porting::getSupportedVideoDrivers();
 	u32 i;

diff --git a/src/client/tile.cpp b/src/client/tile.cpp
@@ -134,9 +134,8 @@ std::string getTexturePath(const std::string &filename)
 	/*
 		Check from texture_path
 	*/
-	std::string texture_path = g_settings->get("texture_path");
-	if (texture_path != "")
-	{
+	const std::string &texture_path = g_settings->get("texture_path");
+	if (texture_path != "") {
 		std::string testpath = texture_path + DIR_DELIM + filename;
 		// Check all filename extensions. Returns "" if not found.
 		fullpath = getImagePath(testpath);
@@ -1854,7 +1853,7 @@ bool TextureSource::generateImagePart(std::string part_of_name,
 			for (u32 x = 0; x < dim.Width; x++)
 			{
 				video::SColor c = baseimg->getPixel(x, y);
-				c.color ^= mask;	
+				c.color ^= mask;
 				baseimg->setPixel(x, y, c);
 			}
 		}
@@ -2266,15 +2265,16 @@ video::ITexture* TextureSource::getNormalTexture(const std::string &name)
 	if (isKnownSourceImage("override_normal.png"))
 		return getTexture("override_normal.png");
 	std::string fname_base = name;
-	std::string normal_ext = "_normal.png";
+	static const char *normal_ext = "_normal.png";
+	static const uint32_t normal_ext_size = strlen(normal_ext);
 	size_t pos = fname_base.find(".");
 	std::string fname_normal = fname_base.substr(0, pos) + normal_ext;
 	if (isKnownSourceImage(fname_normal)) {
 		// look for image extension and replace it
 		size_t i = 0;
 		while ((i = fname_base.find(".", i)) != std::string::npos) {
 			fname_base.replace(i, 4, normal_ext);
-			i += normal_ext.length();
+			i += normal_ext_size;
 		}
 		return getTexture(fname_base);
 		}

diff --git a/src/drawscene.cpp b/src/drawscene.cpp
@@ -494,7 +494,7 @@ void draw_scene(video::IVideoDriver *driver, scene::ISceneManager *smgr,
 	catch(SettingNotFoundException) {}
 #endif
 
-	std::string draw_mode = g_settings->get("3d_mode");
+	const std::string &draw_mode = g_settings->get("3d_mode");
 
 	smgr->drawAll();
 

diff --git a/src/game.cpp b/src/game.cpp
@@ -4094,7 +4094,7 @@ void Game::updateFrame(ProfilerGraph *graph, RunStats *stats, f32 dtime,
 		Drawing begins
 	*/
 
-	video::SColor skycolor = sky->getSkyColor();
+	const video::SColor &skycolor = sky->getSkyColor();
 
 	TimeTaker tt_draw("mainloop: draw");
 	driver->beginScene(true, true, skycolor);

diff --git a/src/itemdef.cpp b/src/itemdef.cpp
@@ -275,16 +275,16 @@ class CItemDefManager: public IWritableItemDefManager
 		assert(i != m_item_definitions.end());
 		return *(i->second);
 	}
-	virtual std::string getAlias(const std::string &name) const
+	virtual const std::string &getAlias(const std::string &name) const
 	{
 		StringMap::const_iterator it = m_aliases.find(name);
 		if (it != m_aliases.end())
 			return it->second;
 		return name;
 	}
-	virtual std::set<std::string> getAll() const
+	virtual void getAll(std::set<std::string> &result) const
 	{
-		std::set<std::string> result;
+		result.clear();
 		for(std::map<std::string, ItemDefinition *>::const_iterator
 				it = m_item_definitions.begin();
 				it != m_item_definitions.end(); ++it) {
@@ -295,7 +295,6 @@ class CItemDefManager: public IWritableItemDefManager
 				it != m_aliases.end(); ++it) {
 			result.insert(it->first);
 		}
-		return result;
 	}
 	virtual bool isKnown(const std::string &name_) const
 	{

diff --git a/src/itemdef.h b/src/itemdef.h
@@ -100,9 +100,9 @@ class IItemDefManager
 	// Get item definition
 	virtual const ItemDefinition& get(const std::string &name) const=0;
 	// Get alias definition
-	virtual std::string getAlias(const std::string &name) const=0;
+	virtual const std::string &getAlias(const std::string &name) const=0;
 	// Get set of all defined item names and aliases
-	virtual std::set<std::string> getAll() const=0;
+	virtual void getAll(std::set<std::string> &result) const=0;
 	// Check if item is known
 	virtual bool isKnown(const std::string &name) const=0;
 #ifndef SERVER
@@ -126,9 +126,9 @@ class IWritableItemDefManager : public IItemDefManager
 	// Get item definition
 	virtual const ItemDefinition& get(const std::string &name) const=0;
 	// Get alias definition
-	virtual std::string getAlias(const std::string &name) const=0;
+	virtual const std::string &getAlias(const std::string &name) const=0;
 	// Get set of all defined item names and aliases
-	virtual std::set<std::string> getAll() const=0;
+	virtual void getAll(std::set<std::string> &result) const=0;
 	// Check if item is known
 	virtual bool isKnown(const std::string &name) const=0;
 #ifndef SERVER

diff --git a/src/mapblock_mesh.cpp b/src/mapblock_mesh.cpp
@@ -855,8 +855,9 @@ static void updateFastFaceRow(
 			makes_face, p_corrected, face_dir_corrected,
 			lights, tile);
 
-	for(u16 j=0; j<MAP_BLOCKSIZE; j++)
-	{
+	// Unroll this variable which has a significant build cost
+	TileSpec next_tile;
+	for (u16 j = 0; j < MAP_BLOCKSIZE; j++) {
 		// If tiling can be done, this is set to false in the next step
 		bool next_is_different = true;
 
@@ -866,20 +867,19 @@ static void updateFastFaceRow(
 		v3s16 next_p_corrected;
 		v3s16 next_face_dir_corrected;
 		u16 next_lights[4] = {0,0,0,0};
-		TileSpec next_tile;
+
 
 		// If at last position, there is nothing to compare to and
 		// the face must be drawn anyway
-		if(j != MAP_BLOCKSIZE - 1)
-		{
+		if (j != MAP_BLOCKSIZE - 1) {
 			p_next = p + translate_dir;
 
 			getTileInfo(data, p_next, face_dir,
 					next_makes_face, next_p_corrected,
 					next_face_dir_corrected, next_lights,
 					next_tile);
 
-			if(next_makes_face == makes_face
+			if (next_makes_face == makes_face
 					&& next_p_corrected == p_corrected + translate_dir
 					&& next_face_dir_corrected == face_dir_corrected
 					&& next_lights[0] == lights[0]
@@ -894,38 +894,14 @@ static void updateFastFaceRow(
 					&& tile.emissive_light == next_tile.emissive_light) {
 				next_is_different = false;
 				continuous_tiles_count++;
-			} else {
-				/*if(makes_face){
-					g_profiler->add("Meshgen: diff: next_makes_face != makes_face",
-							next_makes_face != makes_face ? 1 : 0);
-					g_profiler->add("Meshgen: diff: n_p_corr != p_corr + t_dir",
-							(next_p_corrected != p_corrected + translate_dir) ? 1 : 0);
-					g_profiler->add("Meshgen: diff: next_f_dir_corr != f_dir_corr",
-							next_face_dir_corrected != face_dir_corrected ? 1 : 0);
-					g_profiler->add("Meshgen: diff: next_lights[] != lights[]",
-							(next_lights[0] != lights[0] ||
-							next_lights[0] != lights[0] ||
-							next_lights[0] != lights[0] ||
-							next_lights[0] != lights[0]) ? 1 : 0);
-					g_profiler->add("Meshgen: diff: !(next_tile == tile)",
-							!(next_tile == tile) ? 1 : 0);
-				}*/
 			}
-			/*g_profiler->add("Meshgen: Total faces checked", 1);
-			if(makes_face)
-				g_profiler->add("Meshgen: Total makes_face checked", 1);*/
-		} else {
-			/*if(makes_face)
-				g_profiler->add("Meshgen: diff: last position", 1);*/
 		}
 
-		if(next_is_different)
-		{
+		if (next_is_different) {
 			/*
 				Create a face if there should be one
 			*/
-			if(makes_face)
-			{
+			if (makes_face) {
 				// Floating point conversion of the position vector
 				v3f pf(p_corrected.X, p_corrected.Y, p_corrected.Z);
 				// Center point of face (kind of)
@@ -957,11 +933,9 @@ static void updateFastFaceRow(
 		makes_face = next_makes_face;
 		p_corrected = next_p_corrected;
 		face_dir_corrected = next_face_dir_corrected;
-		lights[0] = next_lights[0];
-		lights[1] = next_lights[1];
-		lights[2] = next_lights[2];
-		lights[3] = next_lights[3];
-		tile = next_tile;
+		std::memcpy(lights, next_lights, ARRLEN(lights) * sizeof(u16));
+		if (next_is_different)
+			tile = next_tile;
 		p = p_next;
 	}
 }

diff --git a/src/minimap.cpp b/src/minimap.cpp
@@ -105,7 +105,7 @@ void MinimapUpdateThread::doUpdate()
 			// Swap two values in the map using single lookup
 			std::pair<std::map<v3s16, MinimapMapblock*>::iterator, bool>
 			    result = m_blocks_cache.insert(std::make_pair(update.pos, update.data));
-			if (result.second == false) {
+			if (!result.second) {
 				delete result.first->second;
 				result.first->second = update.data;
 			}
@@ -322,13 +322,15 @@ void Minimap::setAngle(f32 angle)
 
 void Minimap::blitMinimapPixelsToImageRadar(video::IImage *map_image)
 {
+	video::SColor c(240, 0, 0, 0);
 	for (s16 x = 0; x < data->map_size; x++)
 	for (s16 z = 0; z < data->map_size; z++) {
 		MinimapPixel *mmpixel = &data->minimap_scan[x + z * data->map_size];
 
-		video::SColor c(240, 0, 0, 0);
 		if (mmpixel->air_count > 0)
 			c.setGreen(core::clamp(core::round32(32 + mmpixel->air_count * 8), 0, 255));
+		else
+			c.setGreen(0);
 
 		map_image->setPixel(x, data->map_size - z - 1, c);
 	}
@@ -337,21 +339,23 @@ void Minimap::blitMinimapPixelsToImageRadar(video::IImage *map_image)
 void Minimap::blitMinimapPixelsToImageSurface(
 	video::IImage *map_image, video::IImage *heightmap_image)
 {
+	// This variable creation/destruction has a 1% cost on rendering minimap
+	video::SColor tilecolor;
 	for (s16 x = 0; x < data->map_size; x++)
 	for (s16 z = 0; z < data->map_size; z++) {
 		MinimapPixel *mmpixel = &data->minimap_scan[x + z * data->map_size];
 
 		const ContentFeatures &f = m_ndef->get(mmpixel->n);
 		const TileDef *tile = &f.tiledef[0];
+
 		// Color of the 0th tile (mostly this is the topmost)
-		video::SColor tilecolor;
 		if(tile->has_color)
 			tilecolor = tile->color;
 		else
 			mmpixel->n.getColor(f, &tilecolor);
+
 		tilecolor.setRed(tilecolor.getRed() * f.minimap_color.getRed() / 255);
-		tilecolor.setGreen(tilecolor.getGreen() * f.minimap_color.getGreen()
-			/ 255);
+		tilecolor.setGreen(tilecolor.getGreen() * f.minimap_color.getGreen() / 255);
 		tilecolor.setBlue(tilecolor.getBlue() * f.minimap_color.getBlue() / 255);
 		tilecolor.setAlpha(240);
 
@@ -391,7 +395,7 @@ video::ITexture *Minimap::getMinimapTexture()
 	if (minimap_mask) {
 		for (s16 y = 0; y < MINIMAP_MAX_SY; y++)
 		for (s16 x = 0; x < MINIMAP_MAX_SX; x++) {
-			video::SColor mask_col = minimap_mask->getPixel(x, y);
+			const video::SColor &mask_col = minimap_mask->getPixel(x, y);
 			if (!mask_col.getAlpha())
 				minimap_image->setPixel(x, y, video::SColor(0,0,0,0));
 		}
@@ -430,7 +434,7 @@ scene::SMeshBuffer *Minimap::getMinimapMeshBuffer()
 	scene::SMeshBuffer *buf = new scene::SMeshBuffer();
 	buf->Vertices.set_used(4);
 	buf->Indices.set_used(6);
-	video::SColor c(255, 255, 255, 255);
+	static const video::SColor c(255, 255, 255, 255);
 
 	buf->Vertices[0] = video::S3DVertex(-1, -1, 0, 0, 0, 1, c, 0, 1);
 	buf->Vertices[1] = video::S3DVertex(-1,  1, 0, 0, 0, 1, c, 0, 0);
@@ -550,15 +554,13 @@ void Minimap::updateActiveMarkers()
 	video::IImage *minimap_mask = data->minimap_shape_round ?
 		data->minimap_mask_round : data->minimap_mask_square;
 
-	std::list<Nametag *> *nametags = client->getCamera()->getNametags();
+	const std::list<Nametag *> &nametags = client->getCamera()->getNametags();
 
 	m_active_markers.clear();
 
-	for (std::list<Nametag *>::const_iterator
-			i = nametags->begin();
-			i != nametags->end(); ++i) {
-		Nametag *nametag = *i;
-		v3s16 pos = floatToInt(nametag->parent_node->getPosition() +
+	for (std::list<Nametag *>::const_iterator i = nametags.begin();
+			i != nametags.end(); ++i) {
+		v3s16 pos = floatToInt((*i)->parent_node->getPosition() +
 			intToFloat(client->getCamera()->getOffset(), BS), BS);
 		pos -= data->pos - v3s16(data->map_size / 2,
 				data->scan_height / 2,
@@ -570,7 +572,7 @@ void Minimap::updateActiveMarkers()
 		}
 		pos.X = ((float)pos.X / data->map_size) * MINIMAP_MAX_SX;
 		pos.Z = ((float)pos.Z / data->map_size) * MINIMAP_MAX_SY;
-		video::SColor mask_col = minimap_mask->getPixel(pos.X, pos.Z);
+		const video::SColor &mask_col = minimap_mask->getPixel(pos.X, pos.Z);
 		if (!mask_col.getAlpha()) {
 			continue;
 		}

diff --git a/src/nodedef.cpp b/src/nodedef.cpp
@@ -1332,12 +1332,13 @@ void CNodeDefManager::removeNode(const std::string &name)
 
 void CNodeDefManager::updateAliases(IItemDefManager *idef)
 {
-	std::set<std::string> all = idef->getAll();
+	std::set<std::string> all;
+	idef->getAll(all);
 	m_name_id_mapping_with_aliases.clear();
-	for (std::set<std::string>::iterator
+	for (std::set<std::string>::const_iterator
 			i = all.begin(); i != all.end(); ++i) {
-		std::string name = *i;
-		std::string convert_to = idef->getAlias(name);
+		const std::string &name = *i;
+		const std::string &convert_to = idef->getAlias(name);
 		content_t id;
 		if (m_name_id_mapping.getId(convert_to, id)) {
 			m_name_id_mapping_with_aliases.insert(

diff --git a/src/profiler.h b/src/profiler.h
@@ -119,39 +119,34 @@ class Profiler
 		u32 minindex, maxindex;
 		paging(m_data.size(), page, pagecount, minindex, maxindex);
 
-		for(std::map<std::string, float>::iterator
-				i = m_data.begin();
-				i != m_data.end(); ++i)
-		{
-			if(maxindex == 0)
+		for (std::map<std::string, float>::const_iterator i = m_data.begin();
+				i != m_data.end(); ++i) {
+			if (maxindex == 0)
 				break;
 			maxindex--;
 
-			if(minindex != 0)
-			{
+			if (minindex != 0) {
 				minindex--;
 				continue;
 			}
 
-			std::string name = i->first;
 			int avgcount = 1;
-			std::map<std::string, int>::iterator n = m_avgcounts.find(name);
-			if(n != m_avgcounts.end()){
+			std::map<std::string, int>::const_iterator n = m_avgcounts.find(i->first);
+			if (n != m_avgcounts.end()) {
 				if(n->second >= 1)
 					avgcount = n->second;
 			}
-			o<<"  "<<name<<": ";
+			o << "  " << i->first << ": ";
 			s32 clampsize = 40;
-			s32 space = clampsize - name.size();
-			for(s32 j=0; j<space; j++)
-			{
-				if(j%2 == 0 && j < space - 1)
-					o<<"-";
+			s32 space = clampsize - i->first.size();
+			for(s32 j = 0; j < space; j++) {
+				if (j % 2 == 0 && j < space - 1)
+					o << "-";
 				else
-					o<<" ";
+					o << " ";
 			}
-			o<<(i->second / avgcount);
-			o<<std::endl;
+			o << (i->second / avgcount);
+			o << std::endl;
 		}
 	}