From 072bbba69aa2528c309b76aaec288bdba52e119c Mon Sep 17 00:00:00 2001 From: Loïc Blot Date: Wed, 22 Mar 2017 21:41:02 +0100 Subject: 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 | 3 +-- src/client/clientlauncher.cpp | 2 +- src/client/tile.cpp | 12 +++++------ src/drawscene.cpp | 2 +- src/game.cpp | 2 +- src/itemdef.cpp | 7 +++---- src/itemdef.h | 8 ++++---- src/mapblock_mesh.cpp | 48 ++++++++++--------------------------------- src/minimap.cpp | 30 ++++++++++++++------------- src/nodedef.cpp | 9 ++++---- src/profiler.h | 33 +++++++++++++---------------- src/remoteplayer.cpp | 4 ++-- src/rollback_interface.cpp | 5 +++-- src/settings.cpp | 29 +------------------------- src/settings.h | 4 +--- src/sky.h | 15 ++++++++------ 16 files changed, 79 insertions(+), 134 deletions(-) (limited to 'src') diff --git a/src/camera.h b/src/camera.h index b5be26718..f57efdf10 100644 --- a/src/camera.h +++ b/src/camera.h @@ -172,8 +172,7 @@ public: void removeNametag(Nametag *nametag); - std::list *getNametags() - { return &m_nametags; } + const std::list &getNametags() { return m_nametags; } void drawNametags(); diff --git a/src/client/clientlauncher.cpp b/src/client/clientlauncher.cpp index 2adac53c2..bdd16205f 100644 --- 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 drivers = porting::getSupportedVideoDrivers(); u32 i; diff --git a/src/client/tile.cpp b/src/client/tile.cpp index 000c766d0..85d388d6e 100644 --- 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,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); } diff --git a/src/drawscene.cpp b/src/drawscene.cpp index 3a03743ee..663c8828c 100644 --- 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 index 5b49d34c0..9d52e4326 100644 --- 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 index f43e5c970..627ad1b6c 100644 --- a/src/itemdef.cpp +++ b/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 getAll() const + virtual void getAll(std::set &result) const { - std::set result; + result.clear(); for(std::map::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 { diff --git a/src/itemdef.h b/src/itemdef.h index 2ade6116a..01ec4fa2f 100644 --- a/src/itemdef.h +++ b/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 getAll() const=0; + virtual void getAll(std::set &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 getAll() const=0; + virtual void getAll(std::set &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 index f76033ea8..eddb061b4 100644 --- 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; jadd("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 index ee29c58ba..a7f4822c9 100644 --- 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::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 *nametags = client->getCamera()->getNametags(); + const std::list &nametags = client->getCamera()->getNametags(); m_active_markers.clear(); - for (std::list::const_iterator - i = nametags->begin(); - i != nametags->end(); ++i) { - Nametag *nametag = *i; - v3s16 pos = floatToInt(nametag->parent_node->getPosition() + + for (std::list::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 index 3532eea1e..2745a45e8 100644 --- 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 all = idef->getAll(); + std::set all; + idef->getAll(all); m_name_id_mapping_with_aliases.clear(); - for (std::set::iterator + for (std::set::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 index e8eac86b1..6da115972 100644 --- a/src/profiler.h +++ b/src/profiler.h @@ -119,39 +119,34 @@ public: u32 minindex, maxindex; paging(m_data.size(), page, pagecount, minindex, maxindex); - for(std::map::iterator - i = m_data.begin(); - i != m_data.end(); ++i) - { - if(maxindex == 0) + for (std::map::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::iterator n = m_avgcounts.find(name); - if(n != m_avgcounts.end()){ + std::map::const_iterator n = m_avgcounts.find(i->first); + if (n != m_avgcounts.end()) { if(n->second >= 1) avgcount = n->second; } - o<<" "<first << ": "; s32 clampsize = 40; - s32 space = clampsize - name.size(); - for(s32 j=0; jfirst.size(); + for(s32 j = 0; j < space; j++) { + if (j % 2 == 0 && j < space - 1) + o << "-"; else - o<<" "; + o << " "; } - o<<(i->second / avgcount); - o<second / avgcount); + o << std::endl; } } diff --git a/src/remoteplayer.cpp b/src/remoteplayer.cpp index 0a4591410..c8e5b9132 100644 --- a/src/remoteplayer.cpp +++ b/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); diff --git a/src/rollback_interface.cpp b/src/rollback_interface.cpp index 7345c4a7d..40a33a51d 100644 --- a/src/rollback_interface.cpp +++ b/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 { diff --git a/src/settings.cpp b/src/settings.cpp index c4c3c9073..b4083264e 100644 --- a/src/settings.cpp +++ b/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) diff --git a/src/settings.h b/src/settings.h index b19733514..777d0eff5 100644 --- a/src/settings.h +++ b/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; diff --git a/src/sky.h b/src/sky.h index f19891773..17d6c5f73 100644 --- a/src/sky.h +++ b/src/sky.h @@ -56,18 +56,21 @@ public: void update(float m_time_of_day, float time_brightness, float direct_brightness, bool sunlight_seen, CameraMode cam_mode, float yaw, float pitch); - + 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; } - video::SColorf getCloudColor(){ return m_cloudcolor_f; } + + bool getCloudsVisible() { return m_clouds_visible && m_visible; } + const video::SColorf &getCloudColor() { return m_cloudcolor_f; } void setVisible(bool visible){ m_visible = visible; } void setFallbackBgColor(const video::SColor &fallback_bg_color){ -- cgit v1.2.3