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

Add an editor setting to configure number of threads for lightmap baking #52952

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Sep 22, 2021

This can be used to free some CPU cores when baking lightmaps. When using fewer CPU cores, lightmap baking is slower but background tasks aren't slowed down as much.

Lightmap bake speed comparison on Intel Core i7-6700K @ 4.4 GHz (HyperThreading enabled):

8 threads (CPU threads = 8)
  Done baking lightmaps in 00:01:41.

1 thread (CPU threads = 1)
  Done baking lightmaps in 00:04:52.

This closes godotengine/godot-proposals#3335.

@Calinou Calinou requested review from a team as code owners September 22, 2021 23:02
@Calinou Calinou added this to the 3.4 milestone Sep 22, 2021
@Calinou Calinou force-pushed the cpu-lightmapper-num-threads-editor-setting branch 2 times, most recently from 7ec7031 to edefea1 Compare September 22, 2021 23:16
@Listwon
Copy link
Contributor

Listwon commented Sep 23, 2021

I think it should be integer value representing number of threads and not percentage of available logical cores. It's less clear and can be problematic if for some reason you won't detect the proper number of cores (wrong value returned by get_processor_count or special case where it runs better with one more thread than logical cores etc.).

@Calinou
Copy link
Member Author

Calinou commented Sep 23, 2021

@Listwon This is OK if the default is 0 (use all CPU threads), but it won't allow having a setting that always uses slightly less CPU threads in a smart way. For instance, I don't think it's worth leaving a core free if you're on a dual-core CPU; it'll just be too slow. As for using more threads than CPU cores available, I can allow pushing the ratio up to 1.25, which is likely the highest practical value.

It mostly boils down to making shared configuration files more convenient (e.g. between your desktop and laptop which have a different number of CPU threads).

I've never seen get_processor_count() fail; we've always relied on it so far anyway.

@Listwon
Copy link
Contributor

Listwon commented Sep 23, 2021

Isn't the whole point of adding this option not to make it smart, but to allow the user to run more or less threads? The thing is that it's up to the system how to set the affinity, so all of the threads can run on single core even on multicore rigs. I'd leave the "smart" part to OSes and CPUs. In my experience, we've got two different rigs in our office, one with AMD Ryzen 3600 and one with i5 6500. When fully loaded (building something in Unity), Windows starts being unresponsive, lagging the mouse cursor on AMD, while we don't observe it on Intel machine.

@akien-mga
Copy link
Member

akien-mga commented Sep 28, 2021

I'm also not convinced by the ratio approach, I don't think editor settings are commonly shared between hosts, so IMO it would make more sense to show the actual number of cores available on the host, and let users select a given number.

Alternatively, if we really want to go with a smart approach, I'd go with an enum for the values which could make the most sense: All cores, Nearly all cores (-1 if under 8, -2 otherwise), Half the cores, Single core. But IMO letting users choose with real core numbers is simpler.

@Calinou Calinou force-pushed the cpu-lightmapper-num-threads-editor-setting branch from edefea1 to cb9e5e0 Compare September 28, 2021 14:46
@Calinou
Copy link
Member Author

Calinou commented Sep 28, 2021

I changed it to use an absolute number of CPU threads instead. The default value (0) uses all CPU cores, but you can also use a positive value as an absolute number, or a negative value as a number relative to the total number of CPU cores available.

@Calinou Calinou force-pushed the cpu-lightmapper-num-threads-editor-setting branch from cb9e5e0 to 3b45705 Compare September 28, 2021 14:47
@akien-mga
Copy link
Member

./core/os/threaded_array_processor.h: In function 'void thread_process_array(uint32_t, C*, M, U, int)':
./core/os/threaded_array_processor.h:81:84: error: expected ';' before ')' token
   81 |   thread_count = MAX(1, OS::get_singleton()->get_processor_count() + p_num_threads));
      |                                                                                    ^

This can be used to free some CPU cores when baking lightmaps.
When using fewer CPU cores, lightmap baking is slower but background
tasks aren't slowed down as much.
@Calinou Calinou force-pushed the cpu-lightmapper-num-threads-editor-setting branch from 3b45705 to 0e94393 Compare October 5, 2021 16:27
@Calinou
Copy link
Member Author

Calinou commented Oct 5, 2021

./core/os/threaded_array_processor.h: In function 'void thread_process_array(uint32_t, C*, M, U, int)':
./core/os/threaded_array_processor.h:81:84: error: expected ';' before ')' token
   81 |   thread_count = MAX(1, OS::get_singleton()->get_processor_count() + p_num_threads));
      |                                                                                    ^

Fixed 🙂

@akien-mga akien-mga merged commit 3931667 into godotengine:3.x Oct 5, 2021
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants