[wasm] Add support for pthreads. #3627

annxingyuan · 2020-07-20T17:59:50Z

This fixes #3718

Changes

adds support for multithreading in the WASM backend.
deprecates setWasmPath in favor of setWasmPaths which allows users to specify location of multiple WASM binaries.
generates Emscripten modules for vanilla / SIMD in standalone mode so the same module can be used for both vanilla and SIMD.

Bundle sizes:

master

WASM plain
- js bundle size: 200k
- WASM binary size: 155k
WASM SIMD
- js bundle size: 200k
- WASM binary size: 213k

threads_wasm

WASM plain
- js bundle size: 251k
- WASM binary size: 156k
WASM SIMD
- js bundle size: 251k
- WASM binary size: 214k
WASM threaded SIMD
- js bundle size: 251k
- WASM binary size: 230k

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

This change is

annxingyuan · 2020-07-22T14:45:35Z

Hey @Maratyszcza - just wanted to add you as an FYI - no need to fully review :)

Maratyszcza · 2020-07-23T04:06:45Z

tfjs-backend-wasm/src/backend_wasm.ts

+          [`var _scriptDir = undefined; var WasmBackendModuleSimd = ` +
+           wasmFactoryThreadedSimd.toString()],
+          {type: 'text/javascript'});
+      // } else if (simdSupported) {


I think you still need this else if branch. E.g. Chrome Android supports only SIMD, but not threads

Maratyszcza · 2020-07-23T04:08:16Z

tfjs-backend-wasm/src/cc/backend.cc

 }

 size_t xnn_operator_count = 0;
+pthreadpool *threadpool = pthreadpool_create(4);


It is best to not hardcode the number of threads, but get it dynamically from emscripten_num_logical_cores. I suggest to use std::min(emscripten_num_logical_cores(), 4).

tafsiri

Reviewed 16 of 32 files at r1, 17 of 25 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @pyu10055, and @tafsiri)

tfjs-backend-wasm/README.md, line 193 at r3 (raw file):

### Do you support SIMD and multi-threading?
Yes. We take advantage of SIMD and multi-threading wherever they are supported by testing the capabilities of your runtime and serving the appropriate WASM binary. If you intend to serve the WASM binaries from a custom location (via `setWasmPaths`), please note that the SIMD-enabled and threading-enabled binaries are separate from the vanilla binary.

nit: serving => loading.
vanilla binary => regular (or non-simd/non-threaded) wasm binary

tfjs-backend-wasm/scripts/build-wasm.sh, line 33 at r3 (raw file):

# Threaded + SIMD build.
yarn bazel build -c opt //src/cc:tfjs-backend-wasm-threaded-simd.js --config=wasm --copt="-pthread" --copt="-msimd128"

Question, during local development which build is used in testing? I'm guessing it could depend on your browser? Is there a way to split these out so that locally you don't have to build everything for a local test run?

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

function getWasmPath(
    simdSupported: boolean, threadsSupported: boolean, prefix = '') {
  let path = 'tfjs-backend-wasm.wasm';

could this have a more descriptive name, reading this section it looks more like this is wasmModuleName or something like this, and together with a prefix that forms a path.

Related to naming should prefix be called path or even folderPath/wasmModuleFolder for more clarity? I think prefix is too ambiguous.

Also small nit here could you move this below the early return just below.

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

    }

    console.warn(

I think this should be an error. If a user is trying to customize this, they should probably fail fast rather than fallback to something they may not anticipate. Also the if they have custom paths the default location may still not have the expected file.

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

  }

  if (wasmPathPrefix != null) {

Why not just call this function with wasmPathPrefix as the value for prefix? (and default it to empty string), then you can remove the default value from this functions signature. Another confusion that this may help resolve for me is that as I read it, it looks like passing in prefix in factoryConfig.locateFile = (path, prefix) => { part cannot override wasmPathPrefix, makes wonder why its passed around (as opposed to just setting wasmPathPrefix).

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

let wasmPath: string = null;
let wasmPathPrefix: string = null;
let wasmFileMap: {[key: string]: string} = {};

could you enumerate the valid keys? i'm assuming they are some fixed set that the loader will look for.

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

/** @doc {heading: 'Environment', namespace: 'wasm'} */
export function setWasmPaths(
    prefix: string, fileMap: {[key: string]: string} = {},

are prefix and fileMap exclusive params? can you pass both and have that work? If not (which is my suspicion since the doc says its full paths in fileMap), can you take one param that is either a string or your filemap. Then a user does not need to pass null if they want to use fileMap.

tfjs-backend-wasm/src/flags_wasm.ts, line 42 at r3 (raw file):

    new MessageChannel().port1.postMessage(new SharedArrayBuffer(1));
    return WebAssembly.validate(new Uint8Array([
      0, 97, 115, 109, 1, 0,  0,  0, 1, 4, 1,  96, 0,   0,  3, 2, 1,  0, 5,

Just wondering, is there significance to these specific numbers? if so i'd mention that in the comment so that people aren't tempted to change it in future.

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

pthreadpool *threadpool =
    pthreadpool_create(std::min(std::max(num_cores, 1), 4));

nit: could we turn the 4 into a named variable.

annxingyuan

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @Maratyszcza, @pyu10055, and @tafsiri)

tfjs-backend-wasm/README.md, line 193 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit: serving => loading.
vanilla binary => regular (or non-simd/non-threaded) wasm binary

Done

tfjs-backend-wasm/scripts/build-wasm.sh, line 33 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Question, during local development which build is used in testing? I'm guessing it could depend on your browser? Is there a way to split these out so that locally you don't have to build everything for a local test run?

It will depend on your browser, but for the near term it should end up using the regular binary. I added an option yarn test-dev to build only the regular binary.

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

Previously, Maratyszcza (Marat Dukhan) wrote…

I think you still need this else if branch. E.g. Chrome Android supports only SIMD, but not threads

Done

Although now we don't need the else if branch because in standalone mode the same module works for SIMD and vanilla.

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

Previously, tafsiri (Yannick Assogba) wrote…

could this have a more descriptive name, reading this section it looks more like this is wasmModuleName or something like this, and together with a prefix that forms a path.

Related to naming should prefix be called path or even folderPath/wasmModuleFolder for more clarity? I think prefix is too ambiguous.

Also small nit here could you move this below the early return just below.

Done

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

Previously, tafsiri (Yannick Assogba) wrote…

I think this should be an error. If a user is trying to customize this, they should probably fail fast rather than fallback to something they may not anticipate. Also the if they have custom paths the default location may still not have the expected file.

What if the user just wants to supply a custom URL for one of the 3 binaries though?

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

Previously, tafsiri (Yannick Assogba) wrote…

Why not just call this function with wasmPathPrefix as the value for prefix? (and default it to empty string), then you can remove the default value from this functions signature. Another confusion that this may help resolve for me is that as I read it, it looks like passing in prefix in factoryConfig.locateFile = (path, prefix) => { part cannot override wasmPathPrefix, makes wonder why its passed around (as opposed to just setting wasmPathPrefix).

Done

I think it still makes sense to have wasmPathPrefix default to null though, to distinguish between it being unset, and a user setting it to empty string? That way if locateFile is called with a prefix and wasmPathPrefix has not been set, we can pass in the prefix to getPathToWasmBinary.

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

Previously, tafsiri (Yannick Assogba) wrote…

could you enumerate the valid keys? i'm assuming they are some fixed set that the loader will look for.

Done

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

Previously, tafsiri (Yannick Assogba) wrote…

are prefix and fileMap exclusive params? can you pass both and have that work? If not (which is my suspicion since the doc says its full paths in fileMap), can you take one param that is either a string or your filemap. Then a user does not need to pass null if they want to use fileMap.

Done

tafsiri

LGTM

Reviewed 1 of 25 files at r3, 5 of 6 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @Maratyszcza, and @pyu10055)

tfjs-backend-wasm/scripts/build-wasm.sh, line 33 at r3 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

It will depend on your browser, but for the near term it should end up using the regular binary. I added an option yarn test-dev to build only the regular binary.

Thanks!

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

Previously, annxingyuan (Ann Yuan) wrote…

What if the user just wants to supply a custom URL for one of the 3 binaries though?

ah so in this scheme the user can set one custom wasm module file path to whatever they want and the rest will be resolved against a default wasmModuleFolder value? (which i guess is the empty string) and they need to make sure those binaries are at that location? That makes sense.

I wonder if you can warn about this when reading the wasmFileMap. I say this because unless they dev tests in all these environments they will only see one warning (for whatever their main browser is) and may not realize that the others are falling back (or indeed may come into play at all) until they deploy. Just a suggestion for you to think about.

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

Previously, annxingyuan (Ann Yuan) wrote…

Done

So this will give you typescript checking, but you may want to add a javascript run time check that it is one of those values if a fileMap is passed in. This might avoid issues being filed because of typos.

tfjs-backend-wasm/src/flags_wasm.ts, line 42 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Just wondering, is there significance to these specific numbers? if so i'd mention that in the comment so that people aren't tempted to change it in future.

Still curious about this, adding a comment about these may be sufficient.

annxingyuan

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, @Maratyszcza, @pyu10055, and @tafsiri)

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

Previously, tafsiri (Yannick Assogba) wrote…

ah so in this scheme the user can set one custom wasm module file path to whatever they want and the rest will be resolved against a default wasmModuleFolder value? (which i guess is the empty string) and they need to make sure those binaries are at that location? That makes sense.

I wonder if you can warn about this when reading the wasmFileMap. I say this because unless they dev tests in all these environments they will only see one warning (for whatever their main browser is) and may not realize that the others are falling back (or indeed may come into play at all) until they deploy. Just a suggestion for you to think about.

Yeah that makes a lot of sense! Done.

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

Previously, tafsiri (Yannick Assogba) wrote…

So this will give you typescript checking, but you may want to add a javascript run time check that it is one of those values if a fileMap is passed in. This might avoid issues being filed because of typos.

Done

The warning I just added to address the previous comment should take care of this case I think.

tfjs-backend-wasm/src/flags_wasm.ts, line 42 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Still curious about this, adding a comment about these may be sufficient.

Done

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

Previously, tafsiri (Yannick Assogba) wrote…

nit: could we turn the 4 into a named variable.

Done

annxingyuan added 3 commits July 20, 2020 12:24

temp

22e6783

updates

b6fd69f

works

83379cc

googlebot added the cla: yes label Jul 20, 2020

annxingyuan added 15 commits July 21, 2020 10:08

Merge branch 'master' into threads_wasm

8db1970

use tp

6883c17

ctest

e1c2355

generate 3 builds

d644155

fix build

2f83aa1

updates

a661eee

update

49cbfe5

fix feature test

3ee07cc

clean

f075dde

2 builds

5054af0

redcue build

0819bd7

fix

a2c07e7

fix

2c56435

fix comment

b3c69b2

update

418860a

annxingyuan requested a review from Maratyszcza July 22, 2020 14:45

annxingyuan added 2 commits July 22, 2020 13:41

add

1277909

bench

5a90867

Maratyszcza reviewed Jul 23, 2020

View reviewed changes

Maratyszcza approved these changes Jul 23, 2020

View reviewed changes

annxingyuan added 4 commits July 24, 2020 13:58

add thread

c3c77ce

Merge branch 'master' into threads_wasm

649d794

update

06ce3f9

update

cce44fc

annxingyuan added 11 commits August 3, 2020 13:43

docs

4dd92f2

typo

11e4a17

add message

9593878

docsg

c8149d8

docs

74889f2

docs

d49f21e

eocs

de9d82b

clean

d3e297c

lint

18470d5

Merge branch 'master' into threads_wasm

70d62e5

ignore

5feb860

annxingyuan mentioned this pull request Aug 4, 2020

WIP Update xnn #3015

Closed

annxingyuan added 5 commits August 4, 2020 08:01

Merge branch 'master' into threads_wasm

763f25f

update

c9508bf

docs

d7d818f

fix test

9dc8885

fix

840c16b

tafsiri suggested changes Aug 4, 2020

View reviewed changes

annxingyuan added 5 commits August 4, 2020 11:12

use pixel4 benchmarks

3794dc3

Merge branch 'master' into threads_wasm

43fc845

pr comments

e945562

rename

ae85df3

pr comments

57e3487

annxingyuan commented Aug 4, 2020

View reviewed changes

tafsiri approved these changes Aug 5, 2020

View reviewed changes

comments

7075b30

annxingyuan commented Aug 5, 2020

View reviewed changes

annxingyuan merged commit f580dfc into master Aug 5, 2020

annxingyuan deleted the threads_wasm branch August 5, 2020 17:28

annxingyuan mentioned this pull request Aug 11, 2020

[WASM] The tfjs-backend-wasm module try to find simd binary when I specify a wasm file #3771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[wasm] Add support for pthreads. #3627

[wasm] Add support for pthreads. #3627

Uh oh!

annxingyuan commented Jul 20, 2020 •

edited

Loading

Uh oh!

annxingyuan commented Jul 22, 2020

Uh oh!

Maratyszcza Jul 23, 2020

Uh oh!

Maratyszcza Jul 23, 2020

Uh oh!

tafsiri left a comment

Uh oh!

annxingyuan left a comment

Uh oh!

tafsiri left a comment

Uh oh!

annxingyuan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[wasm] Add support for pthreads. #3627

[wasm] Add support for pthreads. #3627

Uh oh!

Conversation

annxingyuan commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Bundle sizes:

Uh oh!

annxingyuan commented Jul 22, 2020

Uh oh!

Maratyszcza Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

Maratyszcza Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

tafsiri left a comment

Choose a reason for hiding this comment

Uh oh!

annxingyuan left a comment

Choose a reason for hiding this comment

Uh oh!

tafsiri left a comment

Choose a reason for hiding this comment

Uh oh!

annxingyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

annxingyuan commented Jul 20, 2020 •

edited

Loading