Add recipe for oneTBB #427
Conversation
|
@georgestagg apologies for the ping, but would it be possible to have a look at this PR? I'd like to fix building/linking for packages using |
There was a problem hiding this comment.
Thank you for the PR and for working on this! My apologies for the delay in getting around to reviewing it.
The Make rules for oneTBB look good to me overall (don't mind the GitHub CI failure for the Docker build).
I can see how we'll need to compile our own oneTBB library for Wasm, rather than using the vendored tbb in RcppParallel, and so I'm okay with adding the library in principle to our list of optional wasm libs.
Feedback:
-
I have added a couple of minor nits as inline comments below.
-
I think there an issue with the current implementation in this PR. I used
make allto include thelibtbb.aandlibtbbmalloc.alibraries in my development version of webR and then attempted to build RcppParallel pointing to the TBB Wasm library. On my machine (macOS), this failed due to duplicate symbols in the libraries. Emscripten's implementation of the Wasm equivalent of static libraries can be a little unforgiving when it comes to issues like that, it may be that the TMB library requires further patching.
Does building RcppParallel work as expected on your machine with this change? If so, what OS are you using?
If we can fix or work around the duplicated symbol problem when compiling RcppParallel while linking to this Wasm build of TBB, I'll be happy to merge this PR.
Build log for my machine:
$ TBB_INC="/Users/georgestagg/work/webr/wasm/include" TBB_LIB="/Users/georgestagg/work/webr/wasm/lib" R
R version 4.4.0 (2024-04-24) -- "Puppy Cup"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: aarch64-apple-darwin20
R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.
R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.
Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.
> rwasm::add_pkg("RcppParallel")
✔ Loading metadata database ... done
Processing 1 package(s).
trying URL 'https://packagemanager.posit.co/cran/latest/src/contrib/RcppParallel_5.1.7.tar.gz'
Content type 'application/x-tar' length 1634152 bytes (1.6 MB)
==================================================
downloaded 1.6 MB
→ The package (0 B) is cached.
ℹ No downloads are needed
✔ : [4.2s]
* installing *source* package ‘RcppParallel’ ...
file ‘configure’ has the wrong MD5 checksum
** using non-staged installation
configure: sh -c "./configure.orig --build=aarch64-apple-darwin20 --host=wasm32-unknown-emscripten ac_cv_func_getrandom=no"
** preparing to configure package 'RcppParallel' ...
*** configured file: 'R/tbb-autodetected.R.in' => 'R/tbb-autodetected.R'
*** configured file: 'src/Makevars.in' => 'src/Makevars'
*** configured file: 'src/install.libs.R.in' => 'src/install.libs.R'
** finished configure for package 'RcppParallel'
** libs
using C++ compiler: ‘emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.47 (431685f05c67f0424c11473cc16798b9587bb536)’
using SDK: ‘MacOSX14.4.sdk’
em++ -std=gnu++17 -DNDEBUG -I../inst/include -I/Users/georgestagg/work/webr/wasm/include -I/Users/georgestagg/work/webr/wasm/include -I/Users/georgestagg/work/webr/R/build/R-4.4.0/build/include -I/Users/georgestagg/work/webr/R/build/R-4.4.0/src/include -s USE_BZIP2=1 -s USE_ZLIB=1 -std=gnu++11 -fPIC -Oz -fPIC -fwasm-exceptions -s SUPPORT_LONGJMP=wasm -DRCPP_DEMANGLER_ENABLED=0 -D__STRICT_ANSI__ -c init.cpp -o init.o
em++ -std=gnu++17 -DNDEBUG -I../inst/include -I/Users/georgestagg/work/webr/wasm/include -I/Users/georgestagg/work/webr/wasm/include -I/Users/georgestagg/work/webr/R/build/R-4.4.0/build/include -I/Users/georgestagg/work/webr/R/build/R-4.4.0/src/include -s USE_BZIP2=1 -s USE_ZLIB=1 -std=gnu++11 -fPIC -Oz -fPIC -fwasm-exceptions -s SUPPORT_LONGJMP=wasm -DRCPP_DEMANGLER_ENABLED=0 -D__STRICT_ANSI__ -c options.cpp -o options.o
em++ -std=gnu++17 -s SIDE_MODULE=1 -s WASM_BIGINT -s ASSERTIONS=1 -L/Users/georgestagg/work/webr/wasm/lib -L/Users/georgestagg/work/webr/wasm/R-4.4.0/lib/R/lib -s USE_BZIP2=1 -s USE_ZLIB=1 -fwasm-exceptions -s SUPPORT_LONGJMP=wasm -Oz -o RcppParallel.so init.o options.o -Wl,-L,/Users/georgestagg/work/webr/wasm/lib -Wl,-rpath,/Users/georgestagg/work/webr/wasm/lib -ltbb -ltbbmalloc
em++: warning: ignoring unsupported linker flag: `-rpath` [-Wlinkflags]
wasm-ld: error: duplicate symbol: __itt__ittapi_global
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_detach_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_create_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_rename_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_destroy_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_prepare_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_cancel_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_acquired_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_sync_releasing_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_suppress_push_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_suppress_pop_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_suppress_mark_range_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_suppress_clear_range_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_fsync_prepare_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_fsync_cancel_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_fsync_acquired_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_fsync_releasing_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_model_site_begin_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_model_site_end_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: duplicate symbol: __itt_model_task_begin_ptr__3_0
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbb.a(itt_notify.cpp.o)
>>> defined in /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a(itt_notify.cpp.o)
wasm-ld: error: too many errors emitted, stopping now (use -error-limit=0 to see all errors)
em++: error: '/Users/georgestagg/emsdk/upstream/bin/wasm-ld -o RcppParallel.so --whole-archive -L/Users/georgestagg/work/webr/wasm/lib -L/Users/georgestagg/work/webr/wasm/R-4.4.0/lib/R/lib init.o options.o -L /Users/georgestagg/work/webr/wasm/lib /Users/georgestagg/work/webr/wasm/lib/libtbb.a /Users/georgestagg/work/webr/wasm/lib/libtbbmalloc.a -L/Users/georgestagg/emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/pic --no-whole-archive -mllvm -combiner-global-alias-analysis=false -mllvm -wasm-enable-sjlj -mllvm -disable-lsr -mllvm -wasm-enable-eh -mllvm -exception-model=wasm --import-memory --strip-debug --export-dynamic --export-if-defined=main --export-if-defined=__get_exception_message --export-if-defined=free --export-if-defined=__cpp_exception --export-if-defined=__cxa_increment_exception_refcount --export-if-defined=__cxa_decrement_exception_refcount --export-if-defined=__thrown_object_from_unwind_exception --export-if-defined=__start_em_asm --export-if-defined=__stop_em_asm --export-if-defined=__start_em_lib_deps --export-if-defined=__stop_em_lib_deps --export-if-defined=__start_em_js --export-if-defined=__stop_em_js --export-if-defined=__main_argc_argv --export-if-defined=__wasm_apply_data_relocs --export-if-defined=fflush --export=__wasm_call_ctors --experimental-pic -shared' failed (returned 1)
make: *** [RcppParallel.so] Error 1
ERROR: compilation failed for package ‘RcppParallel’
Oh interesting, I've been using It looks like the |
|
Ah ha! That makes sense. So I suppose this is a RcppParallel problem in trying to link both versions of the library when cross-compiling for Emscripten, rather than a problem with TBB itself. OK, I will try again with your patched version of RcppParallel when I am back in front of my computer tomorrow. If that all goes well, I'll merge this PR and then we can send a patch upstream to get the Emscripten compatibility into RcppParallel. |
|
Brilliant, thanks! |
|
Also opened an issue upstream with |
|
@georgestagg I found a discussion about a similar error where a flag for disabling the build of Note that the One question about the changes for RcppParallel - would it be possible to update the |
Great! That looks like a reasonable solution to me.
Adding I also think we can skip the copy in I've made some further minor tweaks to your RcppParallel patches at r-wasm/RcppParallel. This version uses I have now been able to successfully compile |
|
Ah, I see. The error remains because my host version of RCppParallel has not changed. It does not link to oneTBB and includes headers such as In other words, this change does not fix StanEstimators. At least, not by itself. Coordination is required on the cross-compiling machine. Perhaps this needs some more thought. |
Hmm that is a tricky one. Could the recipe install both Emscripten static libs and regular shared libs, such that setting |
|
The vendored I updated the r-wasm/RCppParallel@webr fork to do just that, then ran However, at runtime I still see: I wonder if StanEstimators should be linking to oneTBB directly, rather than through RCppParallel, since we set BTW: Somehow the BH R package (a prerequisite here) is 121MB!! That is not a reasonable download. I'm not sure what's happened there, but it's unrelated to this particular issue. Will investigate another day... |
The vendored headers shouldn't need to be touched - since setting TBB_INC results in the system headers getting copied into
That could indicate an issue with the config, since all packages that depend on I can have a look at the RcppParallel config tomorrow |
I'd rather not require users to reinstall their host RCppParallel package and enforce that they set
OK. I'll return here if anything else occurs to me. |
|
It's not the 'cleanest' solution - but could the build call check for a dependency on That way the host |
|
In general, how does the cross-compilation handle situations where a package needs to link against another package and include headers from the other package? Can it be configured to link/include against a cross-compiled/wasm package, instead of a host x86_64 package? |
It cannot currently. The Including the host version of include headers has not caused issues up until this point, probably because for most R packages we've encountered they do not differ over architectures in this way. However, I can see on my machine that when the system RcppParallel includes are added the undefined symbols appear. OTOH manually compiling the object without I am thinking about how to possibly override this, but I cannot easily see a way to stop R adding in those include arguments. I'm beginning to think your prior intuition was the right way to go after all: having the host RcppParallel also link/include the exact same TBB implementation, complied for x86 rather than Wasm. That way the Wasm and host architechture headers would be more compatible. I think the best way to do this would be to include oneTBB in the official webR development docker image, set the Sorry that it seems we're going around in circles, but I really want to make sure we get this right. I have to work on some other things now, but I'll come back to this when I can. |
|
Yeah the different symbols are caused by different headers unfortunately - RcppParallel includes the older "TBB" library, and there were some significant API changes when Intel changed to "oneTBB" - so some symbols are included in the older headers and not in the newer ones (and vice-versa)
Not at all! Linking with the TBB is a universally painful experience
No worries, no rush on my end! |
|
Another one of the Stan devs just let me know that the current oneTBB has a bug which affects static/WASM initialisation, so this will have to wait until the next release anyway |
|
There was a new release of oneTBB the day before yesterday. |
|
A possible solution for the linking/building issue would be to change |
Adds recipe for building oneTBB following their instructions for WASM.
Let me know if I've missed anything, thanks!