Page MenuHomeWildfire Games

Move pathfinder work to a worker (not yet threaded - D14 prerequisite)
AcceptedPublic

Authored by wraitii on May 25 2019, 10:51 PM.

Details

Reviewers
Kuba386
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Split from D14.

This just moves the "async" pathfinding computations to a worker, in order for D14 to be only about the threading stuff.
This code is thus much easier to review as it's strictly moving things around in a still-not-threaded environment.

Test Plan

Compile, check that things still work.

Diff Detail

Event Timeline

Stan added inline comments.
source/simulation2/Simulation2.cpp
597

Should't a boolean be passed ?

source/simulation2/components/CCmpPathfinder.cpp
100

could it be a shared_pointer ? @vladislavbelov said we should avoid raw pointers but I'm not sure when we should use those.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1503/display/redirect

theotherhiveking added inline comments.
source/simulation2/components/CCmpPathfinder.cpp
100

Here is what Herb Sutter has to say about it:

Guideline: Don’t pass a smart pointer as a function parameter unless you want to use or manipulate the smart pointer itself, such as to share or transfer ownership.

Source:
https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/

wraitii added inline comments.May 26 2019, 12:51 PM
source/simulation2/components/CCmpPathfinder.cpp
100

Here I'm just initialising the unique pointer, so using new is legitimate.

wraitii updated this revision to Diff 8159.May 26 2019, 3:49 PM

Re-upload after D1854 is committed, this should apply cleanly.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/Simulation2.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1510/display/redirect

Kuba386 added a subscriber: Kuba386.Jul 6 2019, 9:52 PM
Kuba386 added inline comments.
source/simulation2/Simulation2.cpp
597

Yes, shouldn't it be false?

wraitii added inline comments.Jul 8 2019, 6:46 PM
source/simulation2/Simulation2.cpp
597

I'm not sure what Stan meant by that or you you mean by that?

Kuba386 added inline comments.Jul 13 2019, 2:13 PM
source/simulation2/Simulation2.cpp
597

I mean shouldn't we change:

StartProcessingMoves(); -> StartProcessingMoves(false);

or

void StartProcessingMoves(bool useMax)
{
    ....
}

->

void StartProcessingMoves(bool useMax = false)
{
    ....
}

?

wraitii added inline comments.Jul 20 2019, 2:54 PM
source/simulation2/Simulation2.cpp
597

= false is defined in ICmpPathfinder.h

wraitii updated this revision to Diff 9015.Jul 20 2019, 4:17 PM

Fix a silly mistake that made StartProcessingMoves not actually process moves when using the 'rate limiter'.

Rebased. RC.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/148/display/redirect

wraitii updated this revision to Diff 9018.Jul 20 2019, 7:04 PM

Have m_Workers from the start as a vector of only one item. Don't use unique_ptr.

RC still.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpPathfinder_Common.h
|  34| #include·"graphics/Overlay.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/simulation2/components/tests/test_Pathfinder.h
|  32| class·TestCmpPathfinder·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestCmpPathfinder:' is invalid C code. Use --std or --language to configure the language.

source/simulation2/components/ICmpPathfinder.h
|  34| template<typename·T>·class·Grid;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/simulation2/Simulation2.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/151/display/redirect

wraitii updated this revision to Diff 9041.Jul 21 2019, 7:21 PM

Don't actually check for sorting of paths because the long/short paths are handled separately and it won't work.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpPathfinder.h
|  34| template<typename·T>·class·Grid;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/simulation2/components/tests/test_Pathfinder.h
|  32| class·TestCmpPathfinder·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestCmpPathfinder:' is invalid C code. Use --std or --language to configure the language.

source/simulation2/components/CCmpPathfinder_Common.h
|  34| #include·"graphics/Overlay.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/simulation2/Simulation2.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/175/display/redirect

Kuba386 accepted this revision.Jul 24 2019, 12:22 PM

Tested on Jebel Barkal, Mainland, Lake, Snowflake, with AI and multiplayer.
Everything seems to work.

This revision is now accepted and ready to land.Jul 24 2019, 12:22 PM

Just tested it on several random maps, with AI. No problems found.

elexis added a subscriber: elexis.Sun, Aug 11, 6:57 PM

(m_SameTurnMovesCount comes from rP9665)

source/simulation2/Simulation2.cpp
597

Why not pass it explicitly, so that the reader of the cpp file is indicated that there is an option to be considered here?
Also why is this false here and not true?

What was the purpose of that max from the cited commit, and why does this purpose not apply anymore for threads?

source/simulation2/components/CCmpPathfinder.cpp
100

The magic of emplace is that it forwards the arguments to the constructor.
So if you pass it a finished type, it will call the move constructor or copy constructor if it wouldn't use copy elision.
If you dont pass anything, you'll end up calling the default constructor in place:

m_Workers.emplace_back(PathfinderWorker{}); -> m_Workers.emplace_back();

712

Should yield linker errors if the specialization is not implemented, so no need to make this a runtime error then?

731

m_Results.push_back({req.ticket, req.notify, path});
->
m_Results.emplace_back(req.ticket, req.notify, path);

I think it's equivalent code flow, but the first one relies on copy elision, the second one is ensured to construct in place without copy or move assignment / constructor.

733

(perhaps one can avoid back() pop_back() with an erase idiom, but that sounds perverse)

740

(same)

756
ComputeShortPathAsync(\n
\t...
\t...)
{

?

765

(wondering whether its faster to pass a point with 2 components by ref or those two numbers, probably the latter)

769

Should check with clang that these proxies are inlined:

CC="clang -Rpass=inline" CXX="clang++ -Rpass=inline" make -j3

(Gotta save the log and grep for function name or file + line nr)

798

(copy elision should hold, may verify with clang)

wraitii added inline comments.Sun, Aug 11, 7:07 PM
source/simulation2/Simulation2.cpp
597

This last call should compute all moves, since it's the last call before the next turn. So obviously it can't use the limit.

It already wasn't using the limit before, nothing's changed.

Nothing against explicitly passing the argument, don't care.

source/simulation2/components/CCmpPathfinder.cpp
712

???

756

not sure how we indent in such cases. It's aligned on the parenthesis here.

769

Definitely completely irrelevant, performance wise, either way, but if one wants to, sure.