Optimize Creature Iteration 2: Electric Boogaloo

[prharvey]

Summary

Performance "Optimized creature iteration during monster planning and parrot_at_danger."

Purpose of change

Monster iteration in some parts of the code iterate over all monsters when they only need to iterate over monsters in their reachable zone. This causes slowdowns in high traffic areas.

Describe the solution

Moved the reachability flood fill into creature_tracker so it could be used in the monster planner to iterate over hostile monsters in O(monsters_in_zone) instead of O(all_monsters). Also change parrot_at_danger to use the same method, at it was iterating over all monsters.

Describe alternatives you've considered

This was put into map in a previous PR, and the result was... not satisfactory.

Testing

Added a test, and manually checked to see if killing a monster or moving between areas would cause any errors.

Additional context

Flame before:

After:

This was previously added in PR #69176, but was causing issues and reverted in PR #69399.

[WhiteCloud0123]

Can we switch our thinking about "parrot_at_danger"? No need to iterate anything and apply it only to "attack_target" .So there should be no annoying performance issues.

[prharvey]

Can we switch our thinking about "parrot_at_danger"? No need to iterate anything and apply it only to "attack_target" .So there should be no annoying performance issues.

I got the impression that the "attack" is meant to warn about new nearby danger other than the current target. So applying it to the attack target defeats the point. Whoever added this would have to weigh in though; I could be wrong.

[Maleclypse]

Can we switch our thinking about "parrot_at_danger"? No need to iterate anything and apply it only to "attack_target" .So there should be no annoying performance issues.

I got the impression that the "attack" is meant to warn about new nearby danger other than the current target. So applying it to the attack target defeats the point. Whoever added this would have to weigh in though; I could be wrong.

You've got it right. It has at some points in time only parroted when it could see the PC and be hostile with them. However, the intention is that dogs bark when they see zombies as an example. As long as parrot at danger causes them to speak when they see a hostile monster it is working correctly.

[akrieger]

ASAN failures look relevant.

(~[slow] ~[.],starting_items)=> ==6734==ERROR: AddressSanitizer: heap-use-after-free on address 0x619005fd4628 at pc 0x000003360edd bp 0x7fff86d70270 sp 0x7fff86d70268
(~[slow] ~[.],starting_items)=> READ of size 8 at 0x619005fd4628 thread T0
(~[slow] ~[.],starting_items)=>     #0 0x3360edc in test_multi_spawn(string_id<mtype> const&, int, int, int, time_point const&, std::set<string_id<mtype>, std::less<string_id<mtype> >, std::allocator<string_id<mtype> > > const&, std::map<string_id<mtype>, int, std::less<string_id<mtype> >, std::allocator<std::pair<string_id<mtype> const, int> > >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3360edc)
(~[slow] ~[.],starting_items)=>     #1 0x3357f9e in ____C_A_T_C_H____T_E_S_T____33() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3357f9e)
(~[slow] ~[.],starting_items)=>     #2 0x3a7f418 in Catch::RunContext::invokeActiveTestCase() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7f418)
(~[slow] ~[.],starting_items)=>     #3 0x3a7b51f in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7b51f)
(~[slow] ~[.],starting_items)=>     #4 0x3a79a0c in Catch::RunContext::runTest(Catch::TestCase const&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a79a0c)
(~[slow] ~[.],starting_items)=>     #5 0x3a87044 in Catch::Session::runInternal() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a87044)
(~[slow] ~[.],starting_items)=>     #6 0x3a85621 in Catch::Session::run() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a85621)
(~[slow] ~[.],starting_items)=>     #7 0x3abe663 in main (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3abe663)
(~[slow] ~[.],starting_items)=>     #8 0x7f963d829d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
(~[slow] ~[.],starting_items)=>     #9 0x7f963d829e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
(~[slow] ~[.],starting_items)=>     #10 0x23a4d34 in _start (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x23a4d34)
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> 0x619005fd4628 is located 680 bytes inside of [102](https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/6927864583/job/18842545788?pr=69574#step:17:103)4-byte region [0x619005fd4380,0x619005fd4780)
(~[slow] ~[.],starting_items)=> freed by thread T0 here:
(~[slow] ~[.],starting_items)=>     #0 0x245[152](https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/6927864583/job/18842545788?pr=69574#step:17:153)d in operator delete(void*) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x245152d)
(~[slow] ~[.],starting_items)=>     #1 0x483d9b4 in std::vector<std::__shared_ptr<monster, (__gnu_cxx::_Lock_policy)0>, std::allocator<std::__shared_ptr<monster, (__gnu_cxx::_Lock_policy)0> > >::_M_erase(__gnu_cxx::__normal_iterator<std::__shared_ptr<monster, (__gnu_cxx::_Lock_policy)0>*, std::vector<std::__shared_ptr<monster, (__gnu_cxx::_Lock_policy)0>, std::allocator<std::__shared_ptr<monster, (__gnu_cxx::_Lock_policy)0> > > >) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x483d9b4)
(~[slow] ~[.],starting_items)=>     #2 0x4831b75 in creature_tracker::remove(monster const&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x4831b75)
(~[slow] ~[.],starting_items)=>     #3 0x5e25a07 in monster::try_upgrade(bool) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x5e25a07)
(~[slow] ~[.],starting_items)=>     #4 0x335f5da in test_multi_spawn(string_id<mtype> const&, int, int, int, time_point const&, std::set<string_id<mtype>, std::less<string_id<mtype> >, std::allocator<string_id<mtype> > > const&, std::map<string_id<mtype>, int, std::less<string_id<mtype> >, std::allocator<std::pair<string_id<mtype> const, int> > >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x335f5da)
(~[slow] ~[.],starting_items)=>     #5 0x3357f9e in ____C_A_T_C_H____T_E_S_T____33() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3357f9e)
(~[slow] ~[.],starting_items)=>     #6 0x3a7f418 in Catch::RunContext::invokeActiveTestCase() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7f418)
(~[slow] ~[.],starting_items)=>     #7 0x3a7b51f in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7b51f)
(~[slow] ~[.],starting_items)=>     #8 0x3a79a0c in Catch::RunContext::runTest(Catch::TestCase const&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a79a0c)
(~[slow] ~[.],starting_items)=>     #9 0x3a87044 in Catch::Session::runInternal() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a87044)
(~[slow] ~[.],starting_items)=>     #10 0x3a85621 in Catch::Session::run() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a85621)
(~[slow] ~[.],starting_items)=>     #11 0x3abe663 in main (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3abe663)
(~[slow] ~[.],starting_items)=>     #12 0x7f963d829d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> previously allocated by thread T0 here:
(~[slow] ~[.],starting_items)=>     #0 0x2450ccd in operator new(unsigned long) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x2450ccd)
(~[slow] ~[.],starting_items)=>     #1 0x4d79230 in std::__shared_count<(__gnu_cxx::_Lock_policy)0>::__shared_count<monster, std::allocator<monster>, string_id<mtype> const&>(monster*&, std::_Sp_alloc_shared_tag<std::allocator<monster> >, string_id<mtype> const&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x4d79230)
(~[slow] ~[.],starting_items)=>     #2 0x4c33b73 in game::place_critter_around(string_id<mtype> const&, tripoint const&, int) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x4c33b73)
(~[slow] ~[.],starting_items)=>     #3 0x335ece0 in test_multi_spawn(string_id<mtype> const&, int, int, int, time_point const&, std::set<string_id<mtype>, std::less<string_id<mtype> >, std::allocator<string_id<mtype> > > const&, std::map<string_id<mtype>, int, std::less<string_id<mtype> >, std::allocator<std::pair<string_id<mtype> const, int> > >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x335ece0)
(~[slow] ~[.],starting_items)=>     #4 0x3357f9e in ____C_A_T_C_H____T_E_S_T____33() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3357f9e)
(~[slow] ~[.],starting_items)=>     #5 0x3a7f418 in Catch::RunContext::invokeActiveTestCase() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7f418)
(~[slow] ~[.],starting_items)=>     #6 0x3a7b51f in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a7b51f)
(~[slow] ~[.],starting_items)=>     #7 0x3a79a0c in Catch::RunContext::runTest(Catch::TestCase const&) (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a79a0c)
(~[slow] ~[.],starting_items)=>     #8 0x3a87044 in Catch::Session::runInternal() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a87044)
(~[slow] ~[.],starting_items)=>     #9 0x3a85621 in Catch::Session::run() (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3a85621)
(~[slow] ~[.],starting_items)=>     #10 0x3abe663 in main (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3abe663)
(~[slow] ~[.],starting_items)=>     #11 0x7f963d829d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> SUMMARY: AddressSanitizer: heap-use-after-free (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x3360edc) in test_multi_spawn(string_id<mtype> const&, int, int, int, time_point const&, std::set<string_id<mtype>, std::less<string_id<mtype> >, std::allocator<string_id<mtype> > > const&, std::map<string_id<mtype>, int, std::less<string_id<mtype> >, std::allocator<std::pair<string_id<mtype> const, int> > >&)
(~[slow] ~[.],starting_items)=> Shadow bytes around the buggy address:
(~[slow] ~[.],starting_items)=>   0x0c3280bf2870: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf2880: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf2890: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf28a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf28b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=> =>0x0c3280bf28c0: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf28d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf28e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=>   0x0c3280bf28f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
(~[slow] ~[.],starting_items)=>   0x0c3280bf2900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
(~[slow] ~[.],starting_items)=>   0x0c3280bf2910: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
(~[slow] ~[.],starting_items)=> Shadow byte legend (one shadow byte represents 8 application bytes):
(~[slow] ~[.],starting_items)=>   Addressable:           00
(~[slow] ~[.],starting_items)=>   Partially addressable: 01 02 03 04 05 06 07 
(~[slow] ~[.],starting_items)=>   Heap left redzone:       fa
(~[slow] ~[.],starting_items)=>   Freed heap region:       fd
(~[slow] ~[.],starting_items)=>   Stack left redzone:      f1
(~[slow] ~[.],starting_items)=>   Stack mid redzone:       f2
(~[slow] ~[.],starting_items)=>   Stack right redzone:     f3
(~[slow] ~[.],starting_items)=>   Stack after return:      f5
(~[slow] ~[.],starting_items)=>   Stack use after scope:   f8
(~[slow] ~[.],starting_items)=>   Global redzone:          f9
(~[slow] ~[.],starting_items)=>   Global init order:       f6
(~[slow] ~[.],starting_items)=>   Poisoned by user:        f7
(~[slow] ~[.],starting_items)=>   Container overflow:      fc
(~[slow] ~[.],starting_items)=>   Array cookie:            ac
(~[slow] ~[.],starting_items)=>   Intra object redzone:    bb
(~[slow] ~[.],starting_items)=>   ASan internal:           fe
(~[slow] ~[.],starting_items)=>   Left alloca redzone:     ca
(~[slow] ~[.],starting_items)=>   Right alloca redzone:    cb
(~[slow] ~[.],starting_items)=>   Shadow gap:              cc
(~[slow] ~[.],starting_items)=> ==6734==ABORTING

[prharvey]

ASAN failures look relevant.

Got a little overzealous when deleting the old faction map stuff, should be good now.

[akrieger]

I tried concocting a contrived situation with clairvoyant monsters inside brick walls that hate each other, but couldn't make a situation that behaved obviously differently before than after.

Tracing a 1hr wait in a tcl blindfolded and earplugged did show about a net 16% improvement, based on exactly one before and after sample which may depend on how monster combat plays out, but it's pretty significant regardless.

When I dug into it though, the savings were a little odd. There's some savings in monster::plan, but monster::move had a larger amount of savings, through faster routing. That seems to imply that the different planning logic is resulting monsters choosing things that are cheaper to route to? Which is a divergence in behavior? Curious to hear your thoughts (or if I should just run more perf traces).

[prharvey]

I tried concocting a contrived situation with clairvoyant monsters inside brick walls that hate each other, but couldn't make a situation that behaved obviously differently before than after.

Tracing a 1hr wait in a tcl blindfolded and earplugged did show about a net 16% improvement, based on exactly one before and after sample which may depend on how monster combat plays out, but it's pretty significant regardless.

When I dug into it though, the savings were a little odd. There's some savings in monster::plan, but monster::move had a larger amount of savings, through faster routing. That seems to imply that the different planning logic is resulting monsters choosing things that are cheaper to route to? Which is a divergence in behavior? Curious to hear your thoughts (or if I should just run more perf traces).

The difference is due to the fact that this optimization is also applied to parrot_at_danger, which is a move that monsters use. It is the smaller box in the PR description flame graph.

[akrieger]

The savings in my traces from parrot_at_danger is only ~1.4k samples, but monster::move as a whole saved ~6.3k samples. I'll take a few more traces and see if there's wider than expected variance here.

[akrieger]

Took two more before & afters. monster::move bounces too much to not be noise. However monster::plan is consistently dropping at least 1/3, at best 43%. Overall about a 15% speedup including the parrot_at_danger improvement.

[Maleclypse]

Shamrock-trimmed.tar.gz

I'm having an issue where I've lost most of the mi-go incredibly easy and they won't respond to any noise I make such as yelling. I'll go test they still work normally in current experimental

edit: Mi-go were much more successful at tracking me after losing sight of me in current experimental. Can someone else try my save and check that?

[Maleclypse]

Shamrock-trimmed.tar.gz

I'm having an issue where I've lost most of the mi-go incredibly easy and they won't respond to any noise I make such as yelling. I'll go test they still work normally in current experimental

edit: Mi-go were much more successful at tracking me after losing sight of me in current experimental. Can someone else try my save and check that?

Nope apparently once i got out of debug mode and stopped using a quickstart debug character everything started working much better. Thanks for this!