animetosho / ParPar

High performance PAR2 create client for NodeJS
190 stars 19 forks source link

No multithreading in macOS #7

Closed bgeet closed 6 years ago

bgeet commented 6 years ago

Versions sw_vers

ProductName: Mac OS X ProductVersion: 10.12.6 BuildVersion: 16G29

node --version

v8.4.0

parpar --version

0.2.2

that's e351cc4be3f9f1713cec331cd54ad401b14594b9 also tried git head as in b1dc0245971a96217ea2810505629562e01d109f

Problem parpar -r5% -s 716800 -o test test.mkv

Method used: Shuffle (128 bit), 1 thread

Only 1 thread, was expecting 4.

using -t, --threads even leads to an error: parpar -t 4 -r5% -s 716800 -o test test.mkv

/Users/test/src/ParPar/bin/parpar.js:156 if(argv.t) ParPar.setMaxThreads(argv.t | 0); ^

TypeError: ParPar.setMaxThreads is not a function at Object. (/Users/test/src/ParPar/bin/parpar.js:156:19) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) at tryModuleLoad (module.js:470:12) at Function.Module._load (module.js:462:3) at Function.Module.runMain (module.js:609:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:598:3

animetosho commented 6 years ago

I suspect that OpenMP may not have been included in your build. Unfortunately I don't have OSX to test, but I did a search and apparently OpenMP may not be included with GCC by default on OSX.

Maybe see if this helps: https://stackoverflow.com/questions/29057437/compile-openmp-programs-with-gcc-compiler-on-os-x-yosemite

bgeet commented 6 years ago

Thanks for the pointer.

Went for newer LLVM that includes OpenMP that's in homebrew: http://blog.llvm.org/2015/05/openmp-support_22.html brew install llvm

gave homebrew's LLVM preference over the one already in macOS: PATH="/usr/local/opt/llvm/bin:$PATH"

build ParPar: CXX="clang++ -fopenmp" LDFLAGS="-L/usr/local/opt/llvm/lib" node-gyp rebuild CXX needed as it doesn't seem to use -fopenmpotherwise.

test: parpar -r5% -s 716800 -o test test.mkv

Method used: Shuffle (128 bit), 4 threads

so now it works!

suggestions:

  1. document in README.md
  2. maybe hard-fail the build when OpenMP is missing
  3. investigate why manual -fopenmp is needed. any files/logs you would need?
animetosho commented 6 years ago

Thanks for the info, and good to hear you got it working.

  1. I'll definitely put something in the readme about this - thanks for bringing it to light!
  2. I do want it to be able to be built without OpenMP for cases where one doesn't want that dependency. Perhaps I could emit a better warning if one tries to use the -t parameter, and OpenMP isn't compiled in (I'd expect the curious to try that flag if they're only getting 1 thread)
  3. Are you sure that -fopenmp is required? It's specified in the build config, so one would think that you wouldn't need to manually specify it. Of course, if you're using a non-default C++ compiler, you'll need to use the CXX environment variable to override. I presume that you did try with just CXX=clang++ though?
bgeet commented 6 years ago

Thanks for taking the time.

Are you sure that -fopenmp is required? It's specified in the build config

where exactly is it specified? if it's specified: shouldn't the build fail if that option isn't supported? related: if i set CXX="c++ -fopenmp" node-gyp -v rebuild, it fails with:

g++ -fopenmp '-DNODE_GYP_MODULE_NAME=parpar_gf' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/Users/test/.node-gyp/8.4.0/include/node -I/Users/test/.node-gyp/8.4.0/src -I/Users/test/.node-gyp/8.4.0/deps/uv/include -I/Users/test/.node-gyp/8.4.0/deps/v8/include -I../gf-complete -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -MMD -MF ./Release/.deps/Release/obj.target/parpar_gf/gf.o.d.raw -c -o Release/obj.target/parpar_gf/gf.o ../gf.cc clang: error: unsupported option '-fopenmp' make: *** [Release/obj.target/parpar_gf/gf.o] Error 1

Of course, if you're using a non-default C++ compiler, you'll need to use the CXX environment variable to override.

true. node-gyp -v rebuild says it's using c++, which is a symlink to macOS's clang++. not homebrew's.

so better to make it explicit and use CXX="/usr/local/opt/llvm/bin/clang++ -fopenmp" instead of $PATH trickery.

I presume that you did try with just CXX=clang++ though?

Just double-checked: no multithreading.

animetosho commented 6 years ago

So I managed to get a hacked version of OSX 10.9 installed in a VM.

It seems like the GYP build system requires it's own section for Mac (as opposed to just taking existing C/LDFLAGS) for whatever reason.
If my understanding is correct, the latest code change to binding.gyp should not require a manual -fopenmp flag to be specified. Are you able to confirm this?

bgeet commented 6 years ago

thanks for your effort.

4f800ecc4 broke the build:

CXX(target) Release/obj.target/parpar_gf/gf.o In file included from ../gf.cc:19: ../gf-complete/gf_complete.h:129:113: error: block pointer to non-function type is invalid ...offset, void *src, void dest, gf_val_32_t *val, int bytes, int xor);

with that commit reverted, it does add -fopenmp. but also to

CC(target) Release/obj.target/parpar_gf/md5/md5.o clang: error: unsupported option '-fopenmp'

so had to additionaly set CC="/usr/local/opt/llvm/bin/clang" to avoid that error. then it works fine.

animetosho commented 6 years ago

Urgh, I've made a bit of a mess... Thanks for sticking with me by the way.

The C files don't need OpenMP, so I guess I can just remove the need there. How does the latest code now work for you? (the CC override should no longer be necessary)

bgeet commented 6 years ago

thanks for your continued support.

so for c9b13f4: CXX="/usr/local/opt/llvm/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm/lib" node-gyp -v rebuild uses:

/usr/local/opt/llvm/bin/clang++ '-DNODE_GYP_MODULE_NAME=parpar_gf' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/Users/test/.node-gyp/8.5.0/include/node -I/Users/test/.node-gyp/8.5.0/src -I/Users/test/.node-gyp/8.5.0/deps/uv/include -I/Users/test/.node-gyp/8.5.0/deps/v8/include -I../gf-complete -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -march=native -O3 -Wall -MMD -MF ./Release/.deps/Release/obj.target/parpar_gf/gf.o.d.raw -c -o Release/obj.target/parpar_gf/gf.o ../gf.cc

no -fopenmp, so no multithreading.

judging by c9b13f4 commit message even for your gcc-toolchain you do need to set cflags for some reason. so keep it the same for mac. then it works with: CC="/usr/local/opt/llvm/bin/clang" CXX="/usr/local/opt/llvm/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm/lib" node-gyp rebuild

test: parpar -r5% -s 716800 -o test test.mkv

Method used: Shuffle (256 bit), 4 threads

(didn't notice before, but it's now also 256 bit not 128, so c993638d5b5c7f5f52f15be053b99fed9f730761 was a success.)

animetosho commented 6 years ago

Urgh, I really don't like the Gyp build system...
Alright, attempt #54 (or whatever it is). Hope this does it.

but it's now also 256 bit not 128, so c993638 was a success

Actually that commit should have no visible effect. Most likely, the -march=native flag is now being sent to the compiler, so it's picking up the AVX2 capabilities of your (probably Haswell?) processor.
Though that's still odd, because the Shuffle method itself probably wouldn't have compiled in without the appropriate -march flag. Eh, it works.

bgeet commented 6 years ago

thanks for your work.

Alright, attempt #54 (or whatever it is). Hope this does it.

for latest git head, as in 0c8ee95bdfe20b455a0af1673fa902649dcd6356 works fine with homebrew's llvm: CC="/usr/local/opt/llvm/bin/clang" CXX="/usr/local/opt/llvm/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm/lib" CPPFLAGS="-I/usr/local/opt/llvm/include" node-gyp rebuild added CPPFLAGS since it's best practice. here's the node-gyp log with some warnings: https://gist.github.com/bgeet/8da2b64c072bfdac6429ee7f49bc433b

test: parpar -r5% -s 716800 -o test test.mkv

Method used: Shuffle (256 bit), 4 threads # Most likely, the -march=native flag is now being sent to the compiler, so it's picking up the AVX2 capabilities of your (probably Haswell?) processor.

that flag is prolly from dc0256b1c7d3f6fa172f7330 then. i have a skylake cpu.

the Shuffle method itself probably wouldn't have compiled in without the appropriate -march flag

checked v0.2.2 tag and there's no -march flag

/usr/local/opt/llvm/bin/clang++ '-DNODE_GYP_MODULE_NAME=parpar_gf' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/Users/test/.node-gyp/8.5.0/include/node -I/Users/test/.node-gyp/8.5.0/src -I/Users/test/.node-gyp/8.5.0/deps/uv/include -I/Users/test/.node-gyp/8.5.0/deps/v8/include -I../gf-complete -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -MMD -MF ./Release/.deps/Release/obj.target/parpar_gf/gf.o.d.raw -I/usr/local/opt/llvm/include -c -o Release/obj.target/parpar_gf/gf.o ../gf.cc

minor nitpick: commit bin/parpar.js with chmod +x since git is aware of unix permissions.

animetosho commented 6 years ago

Yay! Thanks for all the info.

Haven't tried building with Node 8 yet - looks like they've deprecated NewFromOneByte (most of the warnings in the build log you posted), which I'll need to do something about.

commit bin/parpar.js with chmod +x since git is aware of unix permissions.

Thanks for the suggestion. I'll give it a try, but I work on a Windows machine at times, so don't know whether it'll stick.