Merge lp:~widelands-dev/widelands/bug-986611-cppcheck_performance-ui into lp:widelands

[GunChleoc]

Fixed performance issues in UI classes that were flagged up by cppcheck and fixed a textdomain issue in wui/mapdata.

Performance issues fixed are:

- Prefer prefix ++ operator in wui/waresdisplay.cc.
- Constructor initialization in ui_fsmenu/base and wui/mapdata.
- Passing parameters per reference.

Fixed performance issues in UI classes that were flagged up by cppcheck and fixed a textdomain issue in wui/mapdata.

Performance issues fixed are:

- Prefer prefix ++ operator in wui/waresdisplay.cc.
- Constructor initialization in ui_fsmenu/base and wui/mapdata.
- Passing parameters per reference.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1207. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/146981830.
Appveyor build 1046. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_performance_ui-1046.

[SirVer (sirver)]

> - Passing parameters per reference.

I agree with all the other changes and do not feel strongly about this either (i.e. I think this can go in - I doubt any of these changes make any noticeable performance change in Widelands though). I just want to add 2c, because there are more complicated issues here:

1) Passing by reference is only more efficient if a reference is smaller than the datatype. For example RGBColor is a struct of 4bytes which is smaller than he size of a reference (which is he size of a pointer) on a 64 bit system. So passing it by value, i.e. copying it is actually more efficient. Same goes for Coord and some others. So some of the changes will actually decrease performance - though it will never matter, it will never show up on any profile since passing parameters is basically never the bottleneck.

2) Passing a type that manages memory and copying it can be more expensive. Example

struct MyClass {
    MyClass(const vector<int>& a) : a_(a) {}
    vector<int> a_;
}

std::vector<int> millions_of_ints;
MyClass klass(millions_of_ints); // Passing by reference here: millions_of_ints will be copied in a_.
// never use millions of ints again.

Here, the compiler needs to copy a to fulfill our contracts. If we change

MyClass klass(std::move(millions_of_ints));

this does not help at all. The constructor takes a const ref, so the compiler still needs to copy - it cannot avoid the copy. While if we write:

struct MyClass {
    MyClass(vector<int> a) : a_(std::move(a)) {} // No new memory is allocated, no copy made
    vector<int> a_;
}

std::vector<int> millions_of_ints;
MyClass klass(std::move(millions_of_ints)); // Moving the vector - the pointer to its allocated memory is just passed on, there is no additional copy or allocation.

This is the most efficient version we can write. And you see that const ref actually did harm here.

Just my 2c - in general const ref is the best choice, but it is more complicated.

review: Approve

[GunChleoc (gunchleoc)]

The vector thing is actually quite counterintuitive - one would expect a reference not to copy all the elements *headdesk*. I'm sure there is some reason for that design, do you know why they did it that way?

I have added your explanation to my notes :)

@bunnybot merge

[SirVer (sirver)]

The way I like to think about this is along these lines:

func(const vector& blub) means 'I use blub, will not change it and you - the caller - retain ownership'. If I (the function) require 'blub' for longer, I need to make a copy.

func(vector blub) means 'I require my own blub. If you do not need yours anymore, use std::move() and I will use it. Otherwise calling me will copy blub.'

But I find the design choices when to use a per-value and when to use a const ref& not always easy. Generally you could say that if a function copies a value ever, using a per-value argument is preferable.

=== modified file 'src/ui_basic/listselect.cc'
--- src/ui_basic/listselect.cc	2016-03-25 17:01:05 +0000
+++ src/ui_basic/listselect.cc	2016-07-24 11:56:05 +0000
@@ -249,7 +249,7 @@
  * index.
  */
 void BaseListselect::set_entry_color
-	(const uint32_t n, const RGBColor col)
+	(const uint32_t n, const RGBColor& col)
 {
 	assert(n < entry_records_.size());
 

=== modified file 'src/ui_basic/listselect.h'
--- src/ui_basic/listselect.h	2016-02-09 21:14:53 +0000
+++ src/ui_basic/listselect.h	2016-07-24 11:56:05 +0000
@@ -73,7 +73,7 @@
 
 	void switch_entries(uint32_t, uint32_t);
 
-	void set_entry_color(uint32_t, RGBColor);
+	void set_entry_color(uint32_t, const RGBColor&);
 
 	uint32_t size () const {return entry_records_.size ();}
 	bool     empty() const {return entry_records_.empty();}

=== modified file 'src/ui_basic/slider.cc'
--- src/ui_basic/slider.cc	2016-03-30 07:20:05 +0000
+++ src/ui_basic/slider.cc	2016-07-24 11:56:05 +0000
@@ -538,7 +538,7 @@
 DiscreteSlider::DiscreteSlider
 	(Panel * const parent,
 	 const int32_t x, const int32_t y, const uint32_t w, const uint32_t h,
-	 const std::vector<std::string> labels_in,
+	 const std::vector<std::string>& labels_in,
 	 uint32_t value_,
 	 const Image* background_picture_id,
 	 const std::string & tooltip_text,

=== modified file 'src/ui_basic/slider.h'
--- src/ui_basic/slider.h	2016-02-03 18:09:15 +0000
+++ src/ui_basic/slider.h	2016-07-24 11:56:05 +0000
@@ -185,7 +185,7 @@
 	DiscreteSlider
 		(Panel * const parent,
 		 const int32_t x, const int32_t y, const uint32_t w, const uint32_t h,
-		 const std::vector<std::string> labels_in,
+		 const std::vector<std::string>& labels_in,
 		 uint32_t value_,
 		 const Image* background_picture_id,
 		 const std::string & tooltip_text = std::string(),

=== modified file 'src/ui_basic/table.h'
--- src/ui_basic/table.h	2016-02-21 09:39:30 +0000
+++ src/ui_basic/table.h	2016-07-24 11:56:05 +0000
@@ -126,7 +126,7 @@
 		const Image* get_picture(uint8_t col) const;
 		const std::string & get_string(uint8_t col) const;
 		void * entry() const {return entry_;}
-		void set_color(const  RGBColor c) {
+		void set_color(const RGBColor& c) {
 			use_clr = true;
 			clr = c;
 		}

=== modified file 'src/ui_fsmenu/base.cc'
--- src/ui_fsmenu/base.cc	2016-02-07 18:10:53 +0000
+++ src/ui_fsmenu/base.cc	2016-07-24 11:56:05 +0000
@@ -48,10 +48,9 @@
  * Args: bgpic  name of the background picture
  */
 FullscreenMenuBase::FullscreenMenuBase(const std::string& bgpic)
-   : UI::Panel(nullptr, 0, 0, g_gr->get_xres(), g_gr->get_yres()) {
-
-	background_image_ = bgpic;
-}
+   : UI::Panel(nullptr, 0, 0, g_gr->get_xres(), g_gr->get_yres()),
+     background_image_(bgpic)
+{}
 
 FullscreenMenuBase::~FullscreenMenuBase()
 {

=== modified file 'src/wui/field_overlay_manager.cc'
--- src/wui/field_overlay_manager.cc	2016-01-28 05:24:34 +0000
+++ src/wui/field_overlay_manager.cc	2016-07-24 11:56:05 +0000
@@ -117,7 +117,7 @@
 }
 
 void FieldOverlayManager::register_overlay
-	(Widelands::TCoords<> const c,
+	(const Widelands::TCoords<>& c,
 	 const Image* pic,
 	 int32_t              const level,
 	 Point                      hotspot,

=== modified file 'src/wui/field_overlay_manager.h'
--- src/wui/field_overlay_manager.h	2016-01-24 20:11:53 +0000
+++ src/wui/field_overlay_manager.h	2016-07-24 11:56:05 +0000
@@ -85,7 +85,7 @@
 	/// Register an overlay at a location (node or triangle). hotspot is the point
 	/// of the picture that will be exactly over the location. If hotspot is
 	/// Point::invalid(), the center of the picture will be used as hotspot.
-	void register_overlay(const Widelands::TCoords<> coords,
+	void register_overlay(const Widelands::TCoords<>& coords,
 	                      const Image* pic,
 	                      int32_t level,
 	                      Point hotspot = Point::invalid(),

=== modified file 'src/wui/game_debug_ui.cc'
--- src/wui/game_debug_ui.cc	2016-05-17 16:40:22 +0000
+++ src/wui/game_debug_ui.cc	2016-07-24 11:56:05 +0000
@@ -467,7 +467,7 @@
 ===============
 */
 void show_field_debug
-	(InteractiveBase & parent, Widelands::Coords const coords)
+	(InteractiveBase& parent, const Widelands::Coords& coords)
 {
 	new FieldDebugWindow(parent, coords);
 }

=== modified file 'src/wui/game_debug_ui.h'
--- src/wui/game_debug_ui.h	2014-09-10 08:55:04 +0000
+++ src/wui/game_debug_ui.h	2016-07-24 11:56:05 +0000
@@ -28,6 +28,6 @@
 
 // Open debug window for the given coordinates
 void show_mapobject_debug(InteractiveBase & parent, Widelands::MapObject &);
-void show_field_debug(InteractiveBase & parent, Widelands::Coords coords);
+void show_field_debug(InteractiveBase & parent, const Widelands::Coords& coords);
 
 #endif  // end of include guard: WL_WUI_GAME_DEBUG_UI_H

=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2016-05-03 08:45:23 +0000
+++ src/wui/interactive_base.h	2016-07-24 11:56:05 +0000
@@ -186,7 +186,7 @@
 	struct SelData {
 		SelData
 			(const bool Freeze = false, const bool Triangles = false,
-			 const Widelands::NodeAndTriangle<> Pos       =
+			 const Widelands::NodeAndTriangle<>& Pos =
 				Widelands::NodeAndTriangle<>
 			 		(Widelands::Coords(0, 0),
 			 		 Widelands::TCoords<>

=== modified file 'src/wui/interactive_gamebase.h'
--- src/wui/interactive_gamebase.h	2016-03-16 09:22:00 +0000
+++ src/wui/interactive_gamebase.h	2016-07-24 11:56:05 +0000
@@ -77,7 +77,7 @@
 
 protected:
 	void draw_overlay(RenderTarget &) override;
-	virtual int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords> c) = 0;
+	virtual int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords>& c) = 0;
 
 	GameMainMenuWindows      main_windows_;
 	ChatProvider           * chat_provider_;

=== modified file 'src/wui/interactive_player.cc'
--- src/wui/interactive_player.cc	2016-04-02 08:28:29 +0000
+++ src/wui/interactive_player.cc	2016-07-24 11:56:05 +0000
@@ -266,7 +266,7 @@
 	return player_number_;
 }
 
-int32_t InteractivePlayer::calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords> c) {
+int32_t InteractivePlayer::calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords>& c) {
 	assert(get_player());
 	return get_player()->get_buildcaps(c);
 }

=== modified file 'src/wui/interactive_player.h'
--- src/wui/interactive_player.h	2016-01-24 20:11:53 +0000
+++ src/wui/interactive_player.h	2016-07-24 11:56:05 +0000
@@ -77,12 +77,12 @@
 	void cleanup_for_load() override;
 	void think() override;
 
-	void set_flag_to_connect(Widelands::Coords const location) {
+	void set_flag_to_connect(const Widelands::Coords& location) {
 		flag_to_connect_ = location;
 	}
 
 	void popup_message(Widelands::MessageId, const Widelands::Message &);
-	int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords> c) override;
+	int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords>& c) override;
 
 private:
 	void cmdSwitchPlayer(const std::vector<std::string> & args);

=== modified file 'src/wui/interactive_spectator.cc'
--- src/wui/interactive_spectator.cc	2016-03-16 10:32:50 +0000
+++ src/wui/interactive_spectator.cc	2016-07-24 11:56:05 +0000
@@ -143,7 +143,7 @@
 }
 
 
-int32_t InteractiveSpectator::calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords> c) {
+int32_t InteractiveSpectator::calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords>& c) {
 	const Widelands::PlayerNumber nr_players = game().map().get_nrplayers();
 
 	iterate_players_existing(p, nr_players, game(), player) {

=== modified file 'src/wui/interactive_spectator.h'
--- src/wui/interactive_spectator.h	2016-01-24 20:11:53 +0000
+++ src/wui/interactive_spectator.h	2016-07-24 11:56:05 +0000
@@ -51,7 +51,7 @@
 	void toggle_statistics();
 	void exit_btn();
 	void save_btn();
-	int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords> c) override;
+	int32_t calculate_buildcaps(const Widelands::TCoords<Widelands::FCoords>& c) override;
 	bool can_see(Widelands::PlayerNumber) const override;
 	bool can_act(Widelands::PlayerNumber) const override;
 	Widelands::PlayerNumber player_number() const override;

=== modified file 'src/wui/mapdata.cc'
--- src/wui/mapdata.cc	2016-06-07 07:13:03 +0000
+++ src/wui/mapdata.cc	2016-07-24 11:56:05 +0000
@@ -21,42 +21,62 @@
 
 #include "io/filesystem/filesystem.h"
 
-MapData::MapData() : authors(""), nrplayers(0), width(0), height(0),
-		maptype(MapData::MapType::kNormal), displaytype(MapData::DisplayType::kMapnamesLocalized) {}
-
-		MapData::MapData(const Widelands::Map& map, const std::string& init_filename,
-			  const MapData::MapType& init_maptype,
-			  const MapData::DisplayType& init_displaytype) :
-		MapData() {
-		i18n::Textdomain td("maps");
-		filename = init_filename;
-		name = map.get_name().empty() ? _("No Name") : map.get_name();
-		localized_name = _(name);
-		// Localizing this, because some author fields now have "edited by" text.
-		const std::string& author = map.get_author();
-		authors = MapAuthorData(author.empty() ? _("No Author") : _(author));
-		description = map.get_description().empty() ? "" : _(map.get_description());
-		hint = map.get_hint().empty() ? "" : _(map.get_hint());
-		nrplayers = map.get_nrplayers();
-		width = map.get_width();
-		height = map.get_height();
-		suggested_teams = map.get_suggested_teams();
-		tags = map.get_tags();
-		maptype = init_maptype;
-		displaytype = init_displaytype;
-
-		if (maptype == MapData::MapType::kScenario) {
-			tags.insert("scenario");
-		}
-	}
+MapData::MapData(const std::string& init_filename,
+                 const std::string& init_localized_name,
+                 const std::string& init_author,
+                 const MapData::MapType& init_maptype,
+                 const MapData::DisplayType& init_displaytype) :
+   filename(init_filename),
+   name(init_localized_name),
+   localized_name(init_localized_name),
+   authors(init_author),
+   description(""),
+   hint(""),
+   nrplayers(0),
+   width(0),
+   height(0),
+   maptype(init_maptype),
+   displaytype(init_displaytype)
+{}
+
+MapData::MapData(const Widelands::Map& map, const std::string& init_filename,
+	  const MapData::MapType& init_maptype,
+	  const MapData::DisplayType& init_displaytype) :
+		MapData(init_filename,
+              _("No Name"),
+              _("No Author"),
+              init_maptype,
+              init_displaytype) {
+
+	i18n::Textdomain td("maps");
+	if (!map.get_name().empty()) {
+		name = map.get_name();
+	}
+	localized_name = _(name);
+	// Localizing this, because some author fields now have "edited by" text.
+	if (!map.get_author().empty()) {
+		authors = map.get_author();
+	}
+	description = map.get_description().empty() ? "" : _(map.get_description());
+	hint = map.get_hint().empty() ? "" : _(map.get_hint());
+	nrplayers = map.get_nrplayers();
+	width = map.get_width();
+	height = map.get_height();
+	suggested_teams = map.get_suggested_teams();
+	tags = map.get_tags();
+
+	if (maptype == MapData::MapType::kScenario) {
+		tags.insert("scenario");
+	}
+}
 
 MapData::MapData(const std::string& init_filename, const std::string& init_localized_name) :
-		MapData() {
-		filename = init_filename;
-		name = init_localized_name;
-		localized_name = init_localized_name;
-		maptype = MapData::MapType::kDirectory;
-	}
+		MapData(init_filename,
+              init_localized_name,
+              "",
+              MapData::MapType::kDirectory,
+              MapData::DisplayType::kMapnamesLocalized)
+{}
 
 bool MapData::compare_names(const MapData& other) {
 	// The parent directory gets special treatment.

=== modified file 'src/wui/mapdata.h'
--- src/wui/mapdata.h	2016-05-15 09:03:01 +0000
+++ src/wui/mapdata.h	2016-07-24 11:56:05 +0000
@@ -71,10 +71,14 @@
 		kMapnamesLocalized
 	};
 
-
-	/// For incomplete data
-	MapData();
-
+private:
+	/// For common properties
+	MapData(const std::string& init_filename,
+	        const std::string& init_localized_name,
+	        const std::string& init_author,
+	        const MapData::MapType& init_maptype,
+	        const MapData::DisplayType& init_displaytype);
+public:
 	/// For normal maps and scenarios
 	MapData(const Widelands::Map& map, const std::string& init_filename,
 			  const MapData::MapType& init_maptype,

=== modified file 'src/wui/plot_area.cc'
--- src/wui/plot_area.cc	2016-05-20 07:01:10 +0000
+++ src/wui/plot_area.cc	2016-07-24 11:56:05 +0000
@@ -491,7 +491,7 @@
  */
 void WuiPlotArea::draw_plot_line
 		(RenderTarget & dst, std::vector<uint32_t> const * dataset,
-		 uint32_t const highest_scale, float const sub, RGBColor const color, int32_t const yoffset)
+		 uint32_t const highest_scale, float const sub, const RGBColor& color, int32_t const yoffset)
 {
 	if (!dataset->empty()) {
 		float posx = get_inner_w() - kSpaceRight;

=== modified file 'src/wui/plot_area.h'
--- src/wui/plot_area.h	2016-05-20 07:01:10 +0000
+++ src/wui/plot_area.h	2016-07-24 11:56:05 +0000
@@ -96,7 +96,7 @@
 	               uint32_t highest_scale);
 	void draw_plot_line
 		(RenderTarget & dst, std::vector<uint32_t> const * dataset,
-		 uint32_t const highest_scale, float const sub, RGBColor const color, int32_t yoffset);
+		 uint32_t const highest_scale, float const sub, const RGBColor& color, int32_t yoffset);
 	uint32_t get_plot_time() const;
 	/// Recalculates the data
 	virtual void update();

=== modified file 'src/wui/vector.h'
--- src/wui/vector.h	2014-07-05 16:41:51 +0000
+++ src/wui/vector.h	2016-07-24 11:56:05 +0000
@@ -39,12 +39,12 @@
 	}
 
 	// vector addition
-	Vector operator+ (Vector const other) const {
+	Vector operator+ (const Vector& other) const {
 		return Vector(x + other.x, y + other.y, z + other.z);
 	}
 
 	// inner product
-	float operator* (Vector const other) const {
+	float operator* (const Vector& other) const {
 		return x * other.x + y * other.y + z * other.z;
 	}
 

=== modified file 'src/wui/waresdisplay.cc'
--- src/wui/waresdisplay.cc	2016-03-08 21:14:48 +0000
+++ src/wui/waresdisplay.cc	2016-07-24 11:56:05 +0000
@@ -464,7 +464,7 @@
 	std::vector<Widelands::DescriptionIndex>::iterator j;
 	Widelands::TribeDescr::WaresOrder order = tribe.wares_order();
 
-	for (i = order.begin(); i != order.end(); i++)
+	for (i = order.begin(); i != order.end(); ++i)
 		for (j = i->begin(); j != i->end(); ++j)
 			if ((c = map.find(*j)) != map.end()) {
 				ret += "<sub width=30 padding=2><p align=center>"

=== modified file 'src/wui/watchwindow.cc'
--- src/wui/watchwindow.cc	2016-04-03 16:18:24 +0000
+++ src/wui/watchwindow.cc	2016-07-24 11:56:05 +0000
@@ -386,7 +386,7 @@
 ===============
 */
 void show_watch_window
-	(InteractiveGameBase & parent, Widelands::Coords const coords)
+	(InteractiveGameBase & parent, const Widelands::Coords& coords)
 {
 	if (g_options.pull_section("global").get_bool("single_watchwin", false)) {
 		if (g_watch_window)

=== modified file 'src/wui/watchwindow.h'
--- src/wui/watchwindow.h	2014-09-10 08:55:04 +0000
+++ src/wui/watchwindow.h	2016-07-24 11:56:05 +0000
@@ -24,6 +24,6 @@
 
 class InteractiveGameBase;
 
-void show_watch_window(InteractiveGameBase &, Widelands::Coords);
+void show_watch_window(InteractiveGameBase &, const Widelands::Coords&);
 
 #endif  // end of include guard: WL_WUI_WATCHWINDOW_H