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

[wasm] Allow users to set threads count at runtime #5727

Merged
merged 7 commits into from
Oct 15, 2021
Merged

Conversation

jinjingforever
Copy link
Collaborator

@jinjingforever jinjingforever commented Oct 14, 2021

  • Change the PTHREAD_POOL_SIZE linker option in cc/BUILD to always be 8. This option will "pre-create" a pool of web workers. The actual number of threads that is used by XNNPACK should be less than this number. Before this PR, this option is set to the number of logical cores, which is slightly problematic because it is evaluated at build time (not run time) so it might differ on different machines that build the wasm package. (Note that the size "8" is consistent with how tfjs-tflite is set up).
  • Add a setThreadsCount API call to allow users to set the actual number of threads to use in XNNPACK threadpool. The thread count will be passed into cc side through a new initWithThreadsCount method when the backend is initialized.
  • Update the backend.cc so that the threadpool is created in initWithThreadCount so that it knows how many threads to use. I also increased the thread count cap to 8 to match the pre-created webworker pool.

Fixes #4972 #5446

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@google-cla google-cla bot added the cla: yes label Oct 14, 2021
@jinjingforever
Copy link
Collaborator Author

yarn test-threaded-simd also passed locally.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks for fixing this issue.

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @mattsoulanille)


tfjs-backend-wasm/src/backend_wasm.ts, line 489 at r1 (raw file):

 */
export function setThreadsCount(numThreads: number) {
  threadsCount = numThreads;

should we also provide getter for the threadsCount?


tfjs-backend-wasm/src/cc/backend.cc, line 107 at r1 (raw file):

  int capped_num_threads = std::min(
      std::min(std::max(count, min_num_threads), max_num_threads), num_cores);
  tfjs::backend::threadpool = pthreadpool_create(capped_num_threads);

how is threadpool used by xnnpack?

Copy link
Collaborator Author

@jinjingforever jinjingforever left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille and @pyu10055)


tfjs-backend-wasm/src/backend_wasm.ts, line 489 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should we also provide getter for the threadsCount?

Great suggestion! Done.


tfjs-backend-wasm/src/cc/backend.cc, line 107 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

how is threadpool used by xnnpack?

It is used in various XNNPACK related operations. For example: https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/src/cc/kernels/AvgPool.cc#L96. I think internally xnnpack will parallelize the calculations on those threads.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @jinjingforever, @mattsoulanille, and @pyu10055)


tfjs-backend-wasm/src/backend_wasm.ts, line 501 at r2 (raw file):

 */
export function getThreadsCount(): number {
  return actualThreadsCount;

throw error if it is call before initialization (=== -1)?

Copy link
Collaborator Author

@jinjingforever jinjingforever left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @mattsoulanille and @pyu10055)


tfjs-backend-wasm/src/backend_wasm.ts, line 501 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

throw error if it is call before initialization (=== -1)?

Good idea. Done

@jinjingforever jinjingforever merged commit 306b577 into master Oct 15, 2021
@jinjingforever jinjingforever deleted the wasmthreads branch October 21, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WASM multithreading starts 4 threads regardless of available CPU cores
2 participants