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 improvements #1395

Merged
merged 6 commits into from
Jun 24, 2024
Merged

Conversation

pavelkumbrasev
Copy link
Contributor

Description

Improved stack calculation (the thread stack should be defined via link flag).
Described current performance limitations of oneTBB.

Fixes # - issue number(s) if exists
#1287

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

@jellychen @SoilRos

Other information

Signed-off-by: pavelkumbrasev <[email protected]>
Signed-off-by: pavelkumbrasev <[email protected]>
@pavelkumbrasev pavelkumbrasev self-assigned this Jun 10, 2024
@pavelkumbrasev pavelkumbrasev linked an issue Jun 10, 2024 that may be closed by this pull request
if (NOT EMSCRIPTEN_WITHOUT_PTHREAD)
set_property(TARGET Threads::Threads PROPERTY INTERFACE_LINK_LIBRARIES "-pthread")
endif()
set(TBB_EMSCRIPTEN_STACK_SIZE 5242880)
Copy link
Contributor Author

@pavelkumbrasev pavelkumbrasev Jun 10, 2024

Choose a reason for hiding this comment

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

Honestly, I'm not sure if it is an optimal stack size. (I took it from Emsripten documentation and I believe it is 5 MB)

Copy link
Contributor

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.

Hmm, that an interesting point. Ideally TBB should propagate TOTAL_STACK if it was set by user application.
But you are right the default should be changed to 64kb.

src/tbb/global_control.cpp Outdated Show resolved Hide resolved
src/tbb/governor.cpp Outdated Show resolved Hide resolved
src/tbb/governor.cpp Outdated Show resolved Hide resolved
@pavelkumbrasev
Copy link
Contributor Author

@SoilRos could you please clarify if proposed WA works for you?

@pavelkumbrasev pavelkumbrasev requested a review from isaevil June 11, 2024 13:04
SoilRos added a commit to dune-copasi/dune-copasi that referenced this pull request Jun 11, 2024
@SoilRos
Copy link
Contributor

SoilRos commented Jun 11, 2024

@pavelkumbrasev thanks for the heads up! I gave it a local try with dune-copasi/dune-copasi@414282d (i.e. using this branch and following point 2. from the limitations sections here) and looking at the activity monitor I do not see an obvious spike on multithreading yet as I see with x86 or ARM targets. But I cannot discard that this is a bug in our side, I'd need to look into this with more detail again. I would say to not make this fixes depending on this if your tests are already working.

WASM_Support.md Outdated Show resolved Hide resolved
WASM_Support.md Outdated Show resolved Hide resolved
WASM_Support.md Outdated Show resolved Hide resolved
WASM_Support.md Outdated Show resolved Hide resolved
WASM_Support.md Outdated Show resolved Hide resolved
WASM_Support.md Outdated Show resolved Hide resolved
@pavelkumbrasev pavelkumbrasev mentioned this pull request Jun 24, 2024
Copy link
Contributor

@isaevil isaevil left a comment

Choose a reason for hiding this comment

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

LGTM

@pavelkumbrasev pavelkumbrasev merged commit d6ade50 into master Jun 24, 2024
22 checks passed
@pavelkumbrasev pavelkumbrasev deleted the dev/pavelkumbrasev/wasm_improvements branch June 24, 2024 09:40
SoilRos added a commit to dune-copasi/dune-copasi that referenced this pull request Jun 27, 2024
SoilRos added a commit to dune-copasi/dune-copasi that referenced this pull request Jun 27, 2024
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.

tbb on wasm always executed on the main thread.
5 participants