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

src/camera.h

@@ -172,8 +172,7 @@ public:
 
 	void removeNametag(Nametag *nametag);
 
-	std::list<Nametag *> *getNametags()
+	const std::list<Nametag *> &getNametags() { return m_nametags; }
-	{ return &m_nametags; }
 
 	void drawNametags();
 

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;

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");
+	const std::string &texture_path = g_settings->get("texture_path");
-	if (texture_path != "")
+	if (texture_path != "") {
-	{
 		std::string testpath = texture_path + DIR_DELIM + filename;
 		// Check all filename extensions. Returns "" if not found.
 		fullpath = getImagePath(testpath);
@@ -2266,7 +2265,8 @@ 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)) {
@@ -2274,7 +2274,7 @@ video::ITexture* TextureSource::getNormalTexture(const std::string &name)
 		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);
 		}

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();
 

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);

src/itemdef.cpp

@@ -275,16 +275,16 @@ public:
 		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 @@ public:
 				it != m_aliases.end(); ++it) {
 			result.insert(it->first);
 		}
-		return result;
 	}
 	virtual bool isKnown(const std::string &name_) const
 	{

src/itemdef.h

@@ -100,9 +100,9 @@ public:
 	// 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 @@ public:
 	// 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

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,12 +867,11 @@ 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,
@@ -879,7 +879,7 @@ static void updateFastFaceRow(
 					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];
+		std::memcpy(lights, next_lights, ARRLEN(lights) * sizeof(u16));
-		lights[1] = next_lights[1];
+		if (next_is_different)
-		lights[2] = next_lights[2];
+			tile = next_tile;
-		lights[3] = next_lights[3];
-		tile = next_tile;
 		p = p_next;
 	}
 }

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()
+		tilecolor.setGreen(tilecolor.getGreen() * f.minimap_color.getGreen() / 255);
-			/ 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
+	for (std::list<Nametag *>::const_iterator i = nametags.begin();
-			i = nametags->begin();
+			i != nametags.end(); ++i) {
-			i != nametags->end(); ++i) {
+		v3s16 pos = floatToInt((*i)->parent_node->getPosition() +
-		Nametag *nametag = *i;
-		v3s16 pos = floatToInt(nametag->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;
 		}

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;
+		const std::string &name = *i;
-		std::string convert_to = idef->getAlias(name);
+		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(

src/profiler.h

@@ -119,39 +119,34 @@ public:
 		u32 minindex, maxindex;
 		paging(m_data.size(), page, pagecount, minindex, maxindex);
 
-		for(std::map<std::string, float>::iterator
+		for (std::map<std::string, float>::const_iterator i = m_data.begin();
-				i = m_data.begin();
+				i != m_data.end(); ++i) {
-				i != m_data.end(); ++i)
+			if (maxindex == 0)
-		{
-			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);
+			std::map<std::string, int>::const_iterator n = m_avgcounts.find(i->first);
-			if(n != m_avgcounts.end()){
+			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();
+			s32 space = clampsize - i->first.size();
-			for(s32 j=0; j<space; j++)
+			for(s32 j = 0; j < space; j++) {
-			{
+				if (j % 2 == 0 && j < space - 1)
-				if(j%2 == 0 && j < space - 1)
+					o << "-";
-					o<<"-";
 				else
-					o<<" ";
+					o << " ";
 			}
-			o<<(i->second / avgcount);
+			o << (i->second / avgcount);
-			o<<std::endl;
+			o << std::endl;
 		}
 	}
 

src/remoteplayer.cpp

@@ -141,7 +141,7 @@ void RemotePlayer::deSerialize(std::istream &is, const std::string &playername,
 
 	m_dirty = true;
 	//args.getS32("version"); // Version field value not used
-	std::string name = args.get("name");
+	const std::string &name = args.get("name");
 	strlcpy(m_name, name.c_str(), PLAYERNAME_SIZE);
 
 	if (sao) {
@@ -167,7 +167,7 @@ void RemotePlayer::deSerialize(std::istream &is, const std::string &playername,
 		} catch (SettingNotFoundException &e) {}
 
 		try {
-			std::string extended_attributes = args.get("extended_attributes");
+			const std::string &extended_attributes = args.get("extended_attributes");
 			Json::Reader reader;
 			Json::Value attr_root;
 			reader.parse(extended_attributes, attr_root);

src/rollback_interface.cpp

@@ -190,7 +190,6 @@ bool RollbackAction::applyRevert(Map *map, InventoryManager *imgr, IGameDef *gam
 		case TYPE_MODIFY_INVENTORY_STACK: {
 			InventoryLocation loc;
 			loc.deSerialize(inventory_location);
-			std::string real_name = gamedef->idef()->getAlias(inventory_stack.name);
 			Inventory *inv = imgr->getInventory(loc);
 			if (!inv) {
 				infostream << "RollbackAction::applyRevert(): Could not get "
@@ -211,10 +210,12 @@ bool RollbackAction::applyRevert(Map *map, InventoryManager *imgr, IGameDef *gam
 					<< inventory_location << std::endl;
 				return false;
 			}
+			
 			// If item was added, take away item, otherwise add removed item
 			if (inventory_add) {
 				// Silently ignore different current item
-				if (list->getItem(inventory_index).name != real_name)
+				if (list->getItem(inventory_index).name !=
+						gamedef->idef()->getAlias(inventory_stack.name))
 					return false;
 				list->takeItem(inventory_index, inventory_stack.count);
 			} else {

src/settings.cpp

@@ -90,33 +90,6 @@ bool Settings::checkValueValid(const std::string &value)
 	return true;
 }
 
-
-std::string Settings::sanitizeName(const std::string &name)
-{
-	std::string n = trim(name);
-
-	for (const char *s = "=\"{}#"; *s; s++)
-		n.erase(std::remove(n.begin(), n.end(), *s), n.end());
-
-	return n;
-}
-
-
-std::string Settings::sanitizeValue(const std::string &value)
-{
-	std::string v(value);
-	size_t p = 0;
-
-	if (v.substr(0, 3) == "\"\"\"")
-		v.erase(0, 3);
-
-	while ((p = v.find("\n\"\"\"")) != std::string::npos)
-		v.erase(p, 4);
-
-	return v;
-}
-
-
 std::string Settings::getMultiline(std::istream &is, size_t *num_lines)
 {
 	size_t lines = 1;
@@ -398,7 +371,7 @@ Settings *Settings::getGroup(const std::string &name) const
 }
 
 
-std::string Settings::get(const std::string &name) const
+const std::string &Settings::get(const std::string &name) const
 {
 	const SettingsEntry &entry = getEntry(name);
 	if (entry.is_group)

src/settings.h

@@ -129,8 +129,6 @@ public:
 
 	static bool checkNameValid(const std::string &name);
 	static bool checkValueValid(const std::string &value);
-	static std::string sanitizeName(const std::string &name);
-	static std::string sanitizeValue(const std::string &value);
 	static std::string getMultiline(std::istream &is, size_t *num_lines=NULL);
 	static void printEntry(std::ostream &os, const std::string &name,
 		const SettingsEntry &entry, u32 tab_depth=0);
@@ -141,7 +139,7 @@ public:
 
 	const SettingsEntry &getEntry(const std::string &name) const;
 	Settings *getGroup(const std::string &name) const;
-	std::string get(const std::string &name) const;
+	const std::string &get(const std::string &name) const;
 	bool getBool(const std::string &name) const;
 	u16 getU16(const std::string &name) const;
 	s16 getS16(const std::string &name) const;

src/sky.h

@@ -59,15 +59,18 @@ public:
 
 	float getBrightness(){ return m_brightness; }
 
-	video::SColor getBgColor(){
+	const video::SColor &getBgColor() const
+	{
 		return m_visible ? m_bgcolor : m_fallback_bg_color;
 	}
-	video::SColor getSkyColor(){
+
+	const video::SColor &getSkyColor() const
+	{
 		return m_visible ? m_skycolor : m_fallback_bg_color;
 	}
 
-	bool getCloudsVisible(){ return m_clouds_visible && m_visible; }
+	bool getCloudsVisible() { return m_clouds_visible && m_visible; }
-	video::SColorf getCloudColor(){ return m_cloudcolor_f; }
+	const video::SColorf &getCloudColor() { return m_cloudcolor_f; }
 
 	void setVisible(bool visible){ m_visible = visible; }
 	void setFallbackBgColor(const video::SColor &fallback_bg_color){