Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use multiple threads to generate navmeshes #2955

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

slipher
Copy link
Contributor

@slipher slipher commented Mar 15, 2024

Some improvements compared to #2944, in particular:

  • keeps code for generating on main thread only for sgame background navgen
  • can cancel navgen if the sgame shuts down so as not to hang the server
  • fixes regression that map-level errors are no longer cached

Also I added menu options.

slipher and others added 6 commits March 15, 2024 16:21
It's not informative to count both verts and tris since our code always
generates 3 verts per triangle.

m_logEnabled is meant for usage by rcContext::log, not doLog.

Co-Authored-by: DolceTriade <vcelestialragev@gmail.com>
- Fix GLSL message displayed at wrong time
- Fix setting wrong loading fraction variable
Preparation for multi-threaded navgen
Navmesh generation can now in parallel in multiple threads. This is
configured by g_bot_navgen_maxThreads (sgame) and cg_navgen_maxThreads
(cgame).

We didn't do this earlier because we had never tried threads in NaCl and
were also considering porting to a WASM engine without thread support.
The code for generating a little bit each frame for background
generation in the sgame is kept around in case we do end up porting to
platforms without multithreading. The cgame and /navgen admin command
don't have the ability to run only on the main thread for the moment,
but it would be really easy to add it if needed.

The progress computation for percentage of the navmesh generated has been
improved - it now closely corresponds to the actual amount of work
instead of falsely assuming that each class has equal difficulty.

Co-Authored-by: DolceTriade <vcelestialragev@gmail.com>

void UnvContext::DoMainThreadTasks()
{
for ( auto &f : mainThreadTasks_ )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be cleared at the end of the function? If not, please comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to only call it once. I think there is a comment elsewhere about that but I went ahead and added a clear() just to avoid suspicious appearances.

@@ -61,43 +83,52 @@ static NavgenStatus::Code CodeForFailedDtStatus(dtStatus status)

static int tileSize = 64;

void NavmeshGenerator::WriteFile() {
if ( d_->status.code == NavgenStatus::TRANSIENT_FAILURE )
void NavmeshGenerator::WriteFile( const NavgenTask &t ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave comments about which functions are main thread only here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to functions which can be called from a non-main thread since there are much fewer of those and they are the ones with restrictions.

continue;
}

Init( mapName );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just init once outside the main loop to make the intent clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@DolceTriade
Copy link
Member

What are the numbers with and without patch for:

  • Anthill
  • cube
  • plat23

I'll help test. Let's see if we can get this into 0.55

@slipher
Copy link
Contributor Author

slipher commented Mar 16, 2024

All right, I tested like this time( ./daemonded.exe -set vm.sgame.type 1 -pakpath /c/unv/Unvanquished/pkg (-set g_bot_navgen_maxThreads 7) +devmap plat23 +navgen all +quit ) for blocking navgen and this time( ./daemonded.exe -set vm.sgame.type 1 -pakpath /c/unv/Unvanquished/pkg -set g_bot_navgen_maxThreads 7 -set logs.level.sgame.navgen debug +devmap plat23 +bot fill 5 ) (plus manually clicking X at the end, so less accurate) for background navgen. Tested with NaCl on MinGW. I have a 4 cores with hyperthreading processor.

Branch Navgen Type plat23 cube anthill
master blocking navgen 0m4.614s 1m18.648s 3m47.651s
master background navgen 0m6.513s 1m20.323s N/A
#2955 blocking navgen (7 threads) 0m2.399s 0m26.809s 1m14.501s
#2955 background navgen (7 threads) 0m2.657s 0m29.411s 1m18.005s

Also experimented a bit to see if the hyperthreading works well or not, I think not really. For example in a 4-threaded Anthill run I got 51542ms and 14083ms for Tyrant while with 7 threads I got 38567ms and 9689ms for Tyrant.

@@ -158,6 +158,9 @@ Cvar::Cvar<float> cg_motionblurMinSpeed("cg_motionblurMinSpeed", "minimum speed
Cvar::Cvar<bool> cg_spawnEffects("cg_spawnEffects", "desaturate world view when dead or spawning", Cvar::NONE, true);

static Cvar::Cvar<bool> cg_navgenOnLoad("cg_navgenOnLoad", "generate navmeshes when starting a local game", Cvar::NONE, true);
static Cvar::Cvar<int> cg_navgenMaxThreads(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider using the max threads possible by default. no reason not to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow it even works in Nacl. I put the default on hardware_concurrency() - 1 since on Windows at least that's the most you can use without making other apps sluggish.

For the server I am leaving it at 1 background thread by default to respect other services which may be running.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test that hardware_concurrency works on a Linux and a Mac system though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that this API works inside and outside Nacl on Linux and Mac.

Copy link
Member

@DolceTriade DolceTriade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I need to get around to testing this on a shit VPS which only has a single core. tbh, I don't think we should expect people to run these under nacl, but we'll solicit feedback from others too about this situation regardless.

@DolceTriade
Copy link
Member

DolceTriade commented Mar 16, 2024

Also experimented a bit to see if the hyperthreading works well or not, I think not really. For example in a 4-threaded Anthill run I got 51542ms and 14083ms for Tyrant while with 7 threads I got 38567ms and 9689ms for Tyrant.

when I get back home, I'll try running these numbers on my system since I have enough cores to run all the classes in parallel. For me, anthill took ~30s. I don't expect any regressions, but let's be double check to be sure about what we're doing here.

@DolceTriade
Copy link
Member

So this looks good to me. I tested on my shit VPS and it seems to work well. We really shouldn't expect people to generate huge maps like anthill on their single core vps.

I'll solicit some more feedback but otherwise and test perhaps on some more maps, but then we should merge this prior to 0.55.

@DolceTriade
Copy link
Member

LGTM. Let's wait for sweet to give this a try and then we can merge.

@sweet235
Copy link
Contributor

My server does not have the means to test this. But this is about local games mostly, and you already tested this.

@DolceTriade
Copy link
Member

Cool, looks ready to merge.

@sweet235
Copy link
Contributor

I am still not sure if adding this complexity is worth it. Even if the code is free of bugs, it will make the server a much more dangerous thing to maintain for any sysadmin. A wrong command might spawn enough threads to grind the whole machine to a halt.

@DolceTriade
Copy link
Member

I am still not sure if adding this complexity is worth it. Even if the code is free of bugs, it will make the server a much more dangerous thing to maintain for any sysadmin. A wrong command might spawn enough threads to grind the whole machine to a halt.

I think that this is a really cool feature that significantly reduces the time it takes to generate navmeshes. I think that there are reasonable safeguards to prevent server stoppages.

@sweet235
Copy link
Contributor

I think that there are reasonable safeguards to prevent server stoppages.

I'm good, my server does nothing else. But once we merge this, people will probably have to put our server in a managed container on machines that do other work too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants