Large amounts of CPU being used on mission::process_all

[harperers]

Describe the bug

Exactly as it it says in the title, mission::process_all is taking up 57.74% of the total cpu, and it seems to go all the way down to item::has_flag with 43.42% of the total cpu and the highest self CPU.

I have no clue what could be causing this.

Steps to reproduce

1. Load the save.
2. Wait for an hour and see how long it takes, or ideally profile it yourself and see that the same process is using up a large amount of cpu.

Expected behavior

I was expecting that mission::process_all shouldn't use this much of the CPU and cause game time to crawl, but then again, what do I know?

Screenshots

Versions and configuration

• OS: Windows
  • OS Version: 10.0.19043.2130 (21H1)
• Game Version: cca0c2b [64-bit]
• Graphics Version: Tiles
• Game Language: English [en]
• Mods loaded: [ Dark Days Ahead [dda], Disable NPC Needs [no_npc_food], No Fungal Growth [no_fungal_growth], Extra Mutated Scenarios [extra_mut_scens], Mining Mod [Mining_Mod], SpeedyDex [speedydex], No Rail Stations [No_Rail_Stations], Massachusetts [MA] ]

Additional context

Save file: https://drive.google.com/file/d/1iNm4xusioK6cGxgTYBujaYTBvSkrlo3g/view?usp=sharing

[akrieger]

That doesn't surprise me at all. All of these functions create massive amounts of temporary vectors and hitting the allocator that often is just inefficient. These readonly functions should return O(1) allocating lazy views but implementing/importing and using such a view type is a nontrivial project. It helps to have concrete examples of where it can help though.

[harperers]

i'm no expert, but does mission::process_all really need to be run once every single turn? it seems like it could just be checked only once an hour for the same result, as it seems no mission use deadlines tighter then that, nor should they need to.

[anothersimulacrum]

That would seem to work poorly with missions such as 'kill X' or 'go here'.

[harperers]

unless i'm completely an idiot it looks like the only purpose of this function is to process the effects of missions with deadlines, given that if the mission doesn't have a deadline all it does is return.

[Zireael07]

Why would 'kill X' or 'go to Y' care about the turn resolution? Once every 15 minutes, or half an hour, or even an hour should be enough

[harperers]

After looking at it a bit more, mission::process is also used for completing missions that dont have an NPC quest giver that can wrap it up for you. And, that is the part of the code that is actually causing this problem with is_complete. This means that having the code run every 15 minutes instead of every turn might not be an idea solution, as for a quest like finding a katana for the urban samurai you would want the quest to complete when you pick up a katana.

Regardless of how efficient the code is made, i still dont think that it should be checking if you have completed a quest every turn. Perhaps this could be done with a system of turning in quests via a menu of some sort, like being able to turn personal quests in via the mission menu itself?

[Zireael07]

@harperers: Personally I'd solve it via some sort of an "on_item_pickup" hook for your katana example. Really, having code that runs every turn should be limited to things that absolutely need to (effects countdown, food, AI)

[harperers]

@harperers: Personally I'd solve it via some sort of an "on_item_pickup" hook for your katana example. Really, having code that runs every turn should be limited to things that absolutely need to (effects countdown, food, AI)

Having the code run every time you pick up an item would barely be an improvement to running every turn, and there are more personal quests objectives then simply finding items.

[Zireael07]

Everything is better than running every turn, and this hook would only run for certain items (not all the items in game)

[RAldrich]

+1 The vast majority of turns are not spent picking up items, unless you're playing some sort of hardcore OCD virtual inventory simulator. Which I know a lot of us are, but still, nowhere near 100% of turns.

[harperers]

so, mission::process_all should run once every time you kill a monster, a specific monster dies, you pickup a relevant item, you travel a map tile, an hour passes, you level up a skill, or you gain a trait? And this would have to be updated every time a personal mission with a new goal type is added.

Alternatively, every mission type could be processed individually depending on their goals, but this would make it more complicated if a new goal type was added.

[Zireael07]

@harperers: Something like that. A flag per mission type would probably work.

[mqrause]

I didn't look at the code, so it might not be a valid idea: Could missions make use of the event bus or something along these lines? A mission subscribes to it with a specific condition and gets notified once it's completed. Might get a bit complicated with item pickup, drop and the various ways of consumption.

[RenechCDDA]

I didn't look at the code, so it might not be a valid idea: Could missions make use of the event bus or something along these lines? A mission subscribes to it with a specific condition and gets notified once it's completed. Might get a bit complicated with item pickup, drop and the various ways of consumption.

There is a huge amount of available conditions because of the MGOAL_CONDITION goal. A full list of conditions can be found on the NPCs doc (https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/NPCs.md). Not sure how easy that would be.

Maybe mission::process_all could be broken up so it would short-circuit on the easy stuff and only run longer complicated checks on missions with dynamic conditions. Or maybe there's an even simpler way that I'm not familiar with (I am not familiar with the implementation of an event bus, and only vaguely familiar with the concept).

[mqrause]

(I am not familiar with the implementation of an event bus, and only vaguely familiar with the concept).

We already have an implementation in this repo which I was referring to. It's used for achievements, which at least from the outside seem like they'd do similar stuff to missions.

[andrei8l]

Looks like this problem exists only for MGOAL_FIND_ITEM (and the unused MGOAL_FIND_ITEM_GROUP) assigned from the "assign_mission" dialogue effect which assigns the mission with no NPC ID. There are only two such missions:

MISSION_INFECTED_START_FIND_ANTIBIOTICS - shouldn't be a problem long term MISSION_REFUGEE_Jenny_GET_PNEUMATICS - it's definitely assigned incorrectly. Once you get the book the mission finishes and the reward spawns out of thin air. This should be ported to MGOAL_NULL like MISSION_ROBOFAC_INTERCOM_1 or turned into a real mission assigned from the usual mission dialogue (with "offer_mission").

Once you complete MISSION_REFUGEE_Jenny_GET_PNEUMATICS in the provided save, mission::process() drops down to nothing:

EDIT: trivial patch to make this better without ruining the real `MGOAL_FIND_ITEM` missions

diff --git a/src/mission.cpp b/src/mission.cpp
index 9c41d5fd02..15ea77b7f3 100644
--- a/src/mission.cpp
+++ b/src/mission.cpp
@@ -524,17 +524,19 @@ bool mission::is_complete( const character_id &_npc_id ) const
                     }
                 }
             };
-            for( const tripoint &p : here.points_in_radius( player_character.pos(), 5 ) ) {
-                if( player_character.sees( p ) ) {
-                    if( here.has_items( p ) && here.accessible_items( p ) ) {
-                        count_items( here.i_at( p ) );
-                    }
-                    if( const cata::optional<vpart_reference> vp =
-                            here.veh_at( p ).part_with_feature( "CARGO", true ) ) {
-                        count_items( vp->vehicle().get_items( vp->part_index() ) );
-                    }
-                    if( found_quantity >= item_count ) {
-                        break;
+            if( npc_id.is_valid() ) {
+                for( const tripoint &p : here.points_in_radius( player_character.pos(), 5 ) ) {
+                    if( player_character.sees( p ) ) {
+                        if( here.has_items( p ) && here.accessible_items( p ) ) {
+                            count_items( here.i_at( p ) );
+                        }
+                        if( const cata::optional<vpart_reference> vp =
+                                here.veh_at( p ).part_with_feature( "CARGO", true ) ) {
+                            count_items( vp->vehicle().get_items( vp->part_index() ) );
+                        }
+                        if( found_quantity >= item_count ) {
+                            break;
+                        }
                     }
                 }
             }

[SurFlurer]

How about only checking the tiles adjacent to the player's location, aka reducing the radius to 1? This ensures no matter the player picks up the target item or not, the mission is completed, as long as the player decides to interact with that item.

[RenechCDDA]

Related mitigation: #71790