emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.8k stars 3.31k forks source link

WASM package does not work when PTHREAD_POOL_SIZE is in CMakeLists #19312

Open L1onKing opened 1 year ago

L1onKing commented 1 year ago

Please include the following in your bug report:

Version of emscripten/emsdk: emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.10 (c3fe57af0504fe24fc4ba697feb8c204f3c80022) clang version 15.0.0 (https://github.com/llvm/llvm-project 8bc29d14273b05b05d5a56e34c07948dc2c770d3) Target: wasm32-unknown-emscripten Thread model: posix InstalledDir: /home/nabeel/Documents/algoface/emsdk/upstream/bi

Objective:

I have C++ code where performance matters a great deal, and single threaded WASM library is not the best form for it. This C++ code has CPU threading and I want to utilize it in WASM package as well, so I need to build a multi threaded package

Issue Description:

The issue is simple - when I include PTHREAD_POOL_SIZE in CMakeLists, WASM package just freezes. From small session of debugging I saw that there is a dependency wasm-instantiate that does not get processed. I thought that maybe there is a problem in my project, so I built a Minimum Reproducing Example and the issue is indeed present

Here is main.cpp:

#include <iostream>

int main() {
    std::cout << "Hello, world!!" << std::endl;
    return 0;
}

Here is CMakeLists.txt:

cmake_minimum_required(VERSION 3.0.0)
project(test)

if(NOT CMAKE_BUILD_TYPE)
    set(CMAKE_BUILD_TYPE Release)
endif()

#******************* definitions 
add_definitions(-std=c++11)
add_definitions(-D_HAS_STD_BYTE=0) # for c++17

FIND_PACKAGE(Threads REQUIRED)

set(COMMON_LIBRARIES
    ${CMAKE_THREAD_LIBS_INIT}
)

message(STATUS "Platform: EMSCRIPTEN")

SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -s EXPORT_NAME='test_module'")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -O3 --llvm-lto 0 --js-opts 1 --llvm-opts 3 --bind -s NO_EXIT_RUNTIME=1")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -s WASM=1 -s FETCH=1 --memory-init-file 0 --no-heap-copy --closure 0")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pthread -s USE_PTHREADS -s PTHREAD_POOL_SIZE=4 -s INITIAL_MEMORY=128mb")

SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -s EXTRA_EXPORTED_RUNTIME_METHODS=['AsciiToString','stringToUTF8','lengthBytesUTF8']")

add_executable(vmt
    ${CMAKE_SOURCE_DIR}/main.cpp
)

target_link_libraries(vmt
    ${COMMON_LIBRARIES}
)

I launch a test server with necessary headers in order to make a browser available for multithreaded WASM library. Just run python script.py your_port in the terminal:

#!/usr/bin/env python3
from http.server import HTTPServer, SimpleHTTPRequestHandler, test
import sys
class CORSRequestHandler (SimpleHTTPRequestHandler):
  def end_headers (self):
    # setup mandatory MIME types
    self.extensions_map.update({
      ".js": "application/javascript",
      ".wasm": "application/wasm",
    });

    # setup CORS and COEP policies
    self.send_header('Cross-Origin-Embedder-Policy', 'require-corp')
    self.send_header('Cross-Origin-Opener-Policy',   'same-origin')
    SimpleHTTPRequestHandler.end_headers(self)

if __name__ == '__main__':
  aPort = int(sys.argv[1]) if len(sys.argv) > 1 else 8000  
  test(CORSRequestHandler, HTTPServer, port=aPort)

If you build a project with this CMakeLists.txt and put it in browser, you will never see log message. BUT if you remove PTHREAD_POOL_SIZE from CMake it will work.

Also one interesting observation - the package without PTHREAD_POOL_SIZE do not allow to initialize a thread in C++ code. Browser gives an error that thread pool is exhausted and I need to include PTHREAD_POOL_SIZE with necessary number of threads.

How to fix it? How to utilize a multithreaded WASM in a proper way? Thank you very much!

Also attaching a screenshot of browser console for a better understanding: Screenshot_39

sbc100 commented 1 year ago

Can you trying building with -O0 and/or -sASSERTIONS? It might give you some clue as to why wasm-instantiate is not completing.

sbc100 commented 1 year ago

BTW, especially with cmake, I recommend using the simpler form of the -s flags without the space after the -s and without the extra quotes and braces (e.g. -sEXTRA_EXPORTED_RUNTIME_METHODS=AsciiToString,stringToUTF8,lengthBytesUTF8

L1onKing commented 1 year ago

Hi @sbc100! Thank you for your attention to this issue.

I tried building with -O0 and -sASSERTIONS and I incorporated your suggestion regarding simpler form of -s flags. There is no difference in the outcome.

Screenshot_44 Please take a look at the top right of the screenshots. You can see a label "Hidden" which increments rapidly. And once the value in "Hidden" label reaches a number divisable by 200, it shows a warning log about wasm-instantiate

sbc100 commented 1 year ago

What browser are you using?

Can you trying running with -sPTHREADS_DEBUG? Can you try without EXPORT_NAME?

Can you run a simple pthread test in your browser? e.g. ./test/runner browser.test_pthread_create --browser=/path/to/browser

L1onKing commented 1 year ago

I am using Google Chrome 113.0.5672.92

@sbc100 - Ok, I just built SDK with PTHREADS_DEBUG and EXPORT_NAME and it worked! I guess it worked... because I see the log which was the objective

Screenshot_45

My initial guess is that because I remove EXPORT_NAME?.. I am going to investigate it a bit more and leave a comment tomorrow here. Thank you!

sbc100 commented 1 year ago

That sounds right. Do you know why have EXPORT_NAME set in the first place? Sounds like there could be bug there when combining EXPORT_NAME with pthreads.

L1onKing commented 1 year ago

@sbc100 Well, it seems that through EXPORT_NAME WASM package gets interacted with. And if I remove from my original project, I have errors about the fact that module is undefined.

But, coming back to MRE. I modified main.cpp in such a way:

#include <iostream>
#include <thread>
#include <chrono>

void backgroundThread()
{
    std::this_thread::sleep_for(std::chrono::seconds(5)); // sleep for 5 seconds
    std::cout << "Background thread has finished sleeping." << std::endl;
}

int main() {
    std::cout << "Starting background thread." << std::endl;
    std::thread t(backgroundThread);
    t.detach(); // detach the thread and let it run in the background

    std::cout << "Main thread has finished." << std::endl;
    return 0;
}

It is built with EXPORT_NAME commented out and PTHREAD_POOL_SIZE=2. And this is the browser console output:

Screenshot_2

As you can see, thread was initialized like it is supposed to, but it doesn't seem to run correctly, because I have never seen a log output "Background thread has finished sleeping."

My question is - did I make some mistake in WASM package build again, or sleep_for function does not perform in WASM package as expected?

sbc100 commented 1 year ago

One problem with the above example is that when you return from main, the whole program will normally exit. You can use emscripten_exit_with_live_runtime() or emscripten_runtime_keepalive_push() to avoid this.

sbc100 commented 1 year ago

@sbc100 Well, it seems that through EXPORT_NAME WASM package gets interacted with. And if I remove from my original project, I have errors about the fact that module is undefined.

If you only have a single module on your page, and you not using MODULARIZE, why not just interact with your module via the name Module? I have been hoping to remove the ability to use EXPORT_NAME without MODULARIZE but I guess that won't be possible if there are users like yourself using it in this way.

L1onKing commented 1 year ago

One problem with the above example is that when you return from main, the whole program will normally exit. You can use emscripten_exit_with_live_runtime() or emscripten_runtime_keepalive_push() to avoid this.

You are right. I am sorry, dumb mistake from my side :)

So I eliminated EXPORT_NAME, adjusted code base and now I have multithreaded WASM which is very nice. But I have a question - how can I still assign a custom name to my WASM package in a way that EXPORT_NAME does? You mentioned MODULARIZE option, is that something I need to consider?

If you have an example of CMake that builds modularized WASM package, please kindly share it with me. Thank you for your help!

sbc100 commented 1 year ago

The thing about using EXPORT_NAME without MODULARIZE, is that, while it does give you that declared name, its not compatible with multiple modules in the same page/script. If you want you module to be isolated in such a way that it can co-exist with other modules on the same page/script and does not pullute the global namespace with its symbols then you have to use MODULARIZE.

Using EXPORT_NAME on its own does not prevent global namespace pollution.

L1onKing commented 1 year ago

@sbc100 So let me summarize: The right way would be to use EXPORT_NAME along with -sMODULARIZE=1. Please let me know if my understanding correct

sbc100 commented 1 year ago

That is my understanding. I have tried to make the usage of -sEXPORT_NAME depend on -sMODULARIZE in the past but I have not yet managed to land that change.

Given that IIRC EXPORT_NAME implies the exporting of a name from somewhere I'm not sure that its meaning could be when there is no modularization, since what would it be exporting from exactly?

L1onKing commented 1 year ago

Hello @sbc100 !

First of all, thank you for your help, I was able to compile multithreaded WASM package and use it. But I encounter another problem - library works perfectly on Chrome and on Edge, but it doesn't work on Firefox and Safari. Basically, Firefox and Safari is unable to call any API of my WASM library like it doesn't exist.

Could you kindly advise how to fix this issue? Small research in the internet gives a bit confusing results, I refuse to believe that some browsers just does not support multithreading in this day and age :)