[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
Continuous integration builds have changed state: Travis build 2745. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/296920388. Appveyor build 2557. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ai_variables_rework-2557. -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands has been updated. Commit Message changed to: Internal AI variables changed where suitable to a) std::deques, b) to static in order to reduce number of memory de/allocations For more details, see: https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
Can you set a commit message above, then merge? -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
OK, so dequeue is fine then - I just wanted to make sure. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
Continuous integration builds have changed state: Travis build 2741. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/296462614. Appveyor build 2553. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ai_variables_rework-2553. -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
I am going to use [] in my next branch I have in my head. And also current iterators can be reworked to [] if I was in a mood :) I looked at the deque issue. First, all those deques are "iterated" - a member is moved from one side to another, but yes, on some of them also "remove in the middle" is done. When I look at biggest ones, it looks like: std::deque<Widelands::BuildableField*> buildable_fields: rotating std::deque<Widelands::MineableField*> mineable_fields:rotating std::deque unusable_fields: rotating So these are only front/back manipulated std::deque flags: rotating, remove in the middle std::deque roads: rotating, remove in the middle std::deque new_flags: rotating, remove in the middle Yes, here some removing in the middle is done. But rotating is regular (every X seconds Y members are rotated). Removing from the middle is as needed (e.g. teritory dissapears so roads and flags are gone) So this is not such conclusive as you think... I saw some benchmarks comparing various operations on vector, list and deque and deque was not that bad at all and usually significantly faster than list -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
Are you using the []? If not, the bonus doesn't gain you anything ;) What I'm looking at is the performance loss when deleting elements from the queue, because you're not using it as a queue - queues are there so you can push and pop, so deleting is much more efficient with lists. Please have a look if you really need to do all that iteration over the container just to delete an element - the iteration might actually lose you what you are trying to gain here. -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
The problem with list is that everytime item is added/removed the memory is allocated/deallocated. The deques and vectors behave differently. And this is the main goal - reduce number of memory de/allocations. Also I saw some benchmarks and std::list were quite slow - depending on what you are doing with them of course. Also, deque can access members via [] and this is nice bonus... I added the static keyword. -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
http://www.cplusplus.com/reference/deque/deque/:
> For operations that involve frequent insertion or removals of elements at
> positions other than the beginning or the end, deques perform worse and have
> less consistent iterators and references than lists and forward lists.
dequeue over vector can give you some gain here, but I don't understand your
decision to replace lists by dequeues, since you're erasing from the middle. If
you only need to iterate a list forwards, you can use a forward_list to save
some memory.
Diff comments:
>
> === modified file 'src/ai/defaultai_warfare.cc'
> --- src/ai/defaultai_warfare.cc 2017-09-29 16:10:25 +
> +++ src/ai/defaultai_warfare.cc 2017-10-31 17:41:52 +
> @@ -948,6 +956,10 @@
> const uint16_t total_score = scores[0] + scores[1] + scores[2];
>
> int32_t inputs[4 * kFNeuronBitSize] = {0};
Variable is not static.
> + // Reseting values as the variable is static
> + for (int i = 0; i < 4 * kFNeuronBitSize; i++) {
> + inputs[i] = 0;
> + }
> inputs[0] = (msites_total < 1) ? 1 : 0;
> inputs[1] = (msites_total < 2) ? 1 : 0;
> inputs[2] = (msites_total < 3) ? 1 : 0;
--
https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
Continuous integration builds have changed state: Travis build 2725. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/295415518. Appveyor build 2537. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ai_variables_rework-2537. -- https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands
TiborB has proposed merging lp:~widelands-dev/widelands/ai_variables_rework
into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/ai_variables_rework/+merge/333041
This change is only about memory allocation for AI functions. The goal is to
reduce intensity how AI allocated and returns memory for its data. I dont have
a proof but on PCs with low memory the game can have problems with allocation,
resulting to crashes. After this changes I BELIEVE the number of crashes is
reduced. Also I believe it has tiny, but measurable positive effect on game
speed.
So what it contains:
1. changing std::lists to std::deque
2. making some vectors static + using reserve()
3. making some of integers static
I believe no harm should be done with this change
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/ai_variables_rework into lp:widelands.
=== modified file 'src/ai/ai_help_structs.h'
--- src/ai/ai_help_structs.h 2017-09-29 16:10:25 +
+++ src/ai/ai_help_structs.h 2017-10-31 17:41:52 +
@@ -388,7 +388,7 @@
explicit EconomyObserver(Widelands::Economy& e);
Widelands::Economy& economy;
- std::list flags;
+ std::deque flags;
int32_t dismantle_grace_time;
};
=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc 2017-09-29 16:10:25 +
+++ src/ai/defaultai.cc 2017-10-31 17:41:52 +
@@ -162,43 +162,43 @@
});
// Subscribe to ShipNotes.
- shipnotes_subscriber_ =
- Notifications::subscribe([this](const NoteShipMessage& note) {
-
- // in a short time between start and late_initialization the player
- // can get notes that can not be processed.
- // It seems that this causes no problem, at least no substantial
- if (player_ == nullptr) {
- return;
- }
- if (note.ship->get_owner()->player_number() != player_->player_number()) {
- return;
- }
-
- switch (note.message) {
-
- case NoteShipMessage::Message::kGained:
- gain_ship(*note.ship, NewShip::kBuilt);
- break;
-
- case NoteShipMessage::Message::kLost:
- for (std::list::iterator i = allships.begin(); i != allships.end(); ++i) {
- if (i->ship == note.ship) {
- allships.erase(i);
- break;
- }
- }
- break;
-
- case NoteShipMessage::Message::kWaitingForCommand:
- for (std::list::iterator i = allships.begin(); i != allships.end(); ++i) {
- if (i->ship == note.ship) {
- i->waiting_for_command_ = true;
- break;
- }
- }
- }
- });
+ shipnotes_subscriber_ = Notifications::subscribe([this](
+ const NoteShipMessage& note) {
+
+ // in a short time between start and late_initialization the player
+ // can get notes that can not be processed.
+ // It seems that this causes no problem, at least no substantial
+ if (player_ == nullptr) {
+ return;
+ }
+ if (note.ship->get_owner()->player_number() != player_->player_number()) {
+ return;
+ }
+
+ switch (note.message) {
+
+ case NoteShipMessage::Message::kGained:
+ gain_ship(*note.ship, NewShip::kBuilt);
+ break;
+
+ case NoteShipMessage::Message::kLost:
+ for (std::deque::iterator i = allships.begin(); i != allships.end(); ++i) {
+if (i->ship == note.ship) {
+ allships.erase(i);
+ break;
+}
+ }
+ break;
+
+ case NoteShipMessage::Message::kWaitingForCommand:
+ for (std::deque::iterator i = allships.begin(); i != allships.end(); ++i) {
+if (i->ship == note.ship) {
+ i->waiting_for_command_ = true;
+ break;
+}
+ }
+ }
+ });
}
DefaultAI::~DefaultAI() {
@@ -1254,7 +1254,8 @@
}
// are we going to count resources now?
- bool resource_count_now = false;
+ static bool resource_count_now = false;
+ resource_count_now = false;
// Testing in first 10 seconds or if last testing was more then 60 sec ago
if (field.last_resources_check_time < 1 ||
field.last_resources_check_time - gametime > 60 * 1000) {
@@ -1445,12 +1446,15 @@
field.unconnected_nearby = false;
// collect information about productionsites nearby
- std::vector immovables;
+ static std::vector immovables;
+ immovables.reserve(50);
+ immovables.clear();
// Search in a radius of range
map.find_immovables(Area(field.coords, kProductionArea + 2), );
// function seems to return duplicates, so we will use serial numbers to filter them out
- std::set unique_serials;
+ static std::set unique_serials;
+ unique_serials.clear();
for (uint32_t i = 0; i < immovables.size(); ++i) {
const BaseImmovable& base_immovable = *immovables.at(i).object;
@@ -1483,8 +1487,10 @@
map.find_immovables(Area(field.coords, actual_enemy_check_area), );
// We are interested in unconnected immovables, but we must be also close to connected ones
- bool any_connected_imm = false;
- bool any_unconnected_imm = fa
